Should Roact warn if a key changes type?

[LPGhatguy]

Roact inherits an optimization from React that makes subtrees with the same key reconcile much more quickly. Instead of traversing that children, we just unmount the old tree and replace it with a new one.

The relevant branch of the reconciler:

roact/lib/Reconciler.lua

Lines 297 to 315 in ddac5ae

	-- If the element changes type, we assume its subtree will be substantially
	-- different. This lets us skip comparisons of a large swath of nodes.
	if oldElement.component ~= newElement.component then
	local parent = instanceHandle._parent
	local key = instanceHandle._key
	local context
	if isStatefulElement(oldElement) then
	context = instanceHandle._instance._context
	else
	context = instanceHandle._context
	end
	Reconciler.unmount(instanceHandle)
	local newInstance = Reconciler._mountInternal(newElement, parent, key, context)
	return newInstance
	end

Problem

This causes some confusing behavior for people who write functions that return components, known as higher-order-components in the React community:

-- A very contrived component
local function makeFoo(frameProps)
	local Foo = Roact.Component:extend("Foo")

	function Foo:render()
		return Roact.createElement("Frame", frameProps)
	end

	return Foo
end

When using makeFoo, the intention is that users would do this:

local someFoo = makeFoo({
	BorderSizePixel = 0,
})

local function App()
	return Roact.createElement(someFoo)
end

However, I've found that users will mistakenly do this:

local function App()
	return Roact.createElement(makeFoo({
		BorderSizePixel = 0,
	}))
end

Proposed Solution

I think it would be acceptable for Roact to keep this branch, but throw a warning when it's encountered. This would alert a user if they're doing something that causes Roact to be wasteful.

There are some cases where previously I would've used this feature to implement something like a tab panel, where the same key would be reused when the panel changes. However, I think that embracing dynamic keys in that case, by varying the key based on which panel is being displayed, has other benefits for making such a tab panel more flexible.

What gives me hesitation on this:

• Are there any components where reusing keys but changing components is a necessary or superior technique?
• Is a warning the right technique here? Should it be part of dev tooling instead?
• Should this be enabled by default? What about on 'production' builds?

[AmaranthineCodices]

This is definitely something that we can catch! This comes up in React as well.

I can think of a few ways that changing component types for the same keys can be useful. One is a button. A simple (though certainly not efficient) technique for disabling the button would be to replace the button with a TextLabel, something like:

local function DisableableButton(props)
    if props.Disabled then
        return Roact.createElement("TextLabel", {
            -- ...
        })
    else
        return Roact.createElement("TextButton", {
            -- ...
        })
    end
end

I think a warning in Roact itself is probably the easiest and best way to implement this. Developer tooling would have to have some sort of time-travel or debugging feature where you could step through a series of Roact renders to really catch this effectively, I think.

This should absolutely be enabled by default; here's my rationale:

• Changing the type of a key is almost always outright wrong, and even when it's not, there's probably a better way to do things.
• The check is really cheap from a performance point of view: it's just a warn statement inside the equality check that Roact already does!
• Failing to enable this flag can open you up to very big potential performance hits if the generated components are very large! It's better to force Roact users to opt-out of this behavior than rely on them to remember to opt into it.

Since this doesn't really have a performance impact, there's not much of a reason to selectively enable this depending on the environment that Roact is running in - I think we can leave it on by default in "production" builds.

[LPGhatguy]

Alright, I'm definitely in the camp of "yes, this should always warn"