Warn when mixing createRoot() and old APIs #14615
Conversation
This way further warning check doesn't crash on bad inputs.
gaearon
force-pushed
the
warn-confusing
branch
from
d3921cc to
bd99d17
Compare
|
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Details of bundled changes.Comparing: 4feab7f...4253819 react-dom
Generated by 🚫 dangerJS |
| warningWithoutStack( | ||
| !container._reactIsNewStyleRootDEV, | ||
| 'You are calling ReactDOM.hydrate() on a container that was previously ' + | ||
| 'managed by ReactDOM.%s(). This is not supported. ' + |
There was a problem hiding this comment.
"created by createRoot" sounded a bit tautological but I don't know if manage is better.
Container also wasn't technically "created" by createRoot. createRoot created the root but it's the container DOM node that we've previously passed to the other API. Not the root.
There was a problem hiding this comment.
I went with "passed to"
| @@ -160,9 +159,11 @@ setRestoreImplementation(restoreControlledState); | |||
| export type DOMContainer = | |||
| | (Element & { | |||
| _reactRootContainer: ?Root, | |||
| _reactIsNewStyleRootDEV: ?boolean, | |||
There was a problem hiding this comment.
this can read as "new StyleRoot" (and confused me for a moment), I assume you mean "new-style root". maybe reactIsCreateRootDEV?
| warningWithoutStack( | ||
| !container._reactIsNewStyleRootDEV, | ||
| 'You are calling ReactDOM.render() on a container that was previously ' + | ||
| 'managed by ReactDOM.%s(). This is not supported. ' + |
gaearon
force-pushed
the
warn-confusing
branch
from
84bd2a3 to
54b3db1
Compare
| @@ -160,9 +159,11 @@ setRestoreImplementation(restoreControlledState); | |||
| export type DOMContainer = | |||
| | (Element & { | |||
| _reactRootContainer: ?Root, | |||
| _reactHasBeenPassedToCreateRootDEV: ?boolean, | |||
There was a problem hiding this comment.
Shouldn't need this because _reactRootContainer only exists on legacy roots.
There was a problem hiding this comment.
My primary goal was to detect that a container previously passed to createRoot is then passed to unmountComponentAtNode. I don't think I can do this without some kind of a marker on the new container.
I can't check its first child because createRoot doesn't clear children (and so could mount at arbitrary point). Scanning all children seems not great. I don't know of another way to detect that a DOM element is a new-style root.
There was a problem hiding this comment.
const reactHasBeenPassedToCreateRoot = domContainer._reactRootContainer === null;There was a problem hiding this comment.
But isn't that used by ReactDOM.render too? Since it's expressed via createRoot internally.
There was a problem hiding this comment.
ReactDOM.render uses the ReactRoot type, but it doesn't call createRoot directly.
The only place _reactRootContainer is set is in legacyRenderSubtreeIntoContainer. New roots don't need the expando because the root is retained in user space.
There was a problem hiding this comment.
I think we're talking about two different cases here.
-
Detecting DOM container that's previously been used by
ReactDOM.render. This is easy. That's what_reactRootContainergives us. -
Detecting DOM container that's previously been used by
ReactDOM.createRoot. I don't see a way to do this currently. As you said, "new roots don't need the expando". That's why I couldn't figure out how to detect them, and added a new DEV field.
I need both checks. Not just one way around. In fact (2) is primary motivation for this PR — since Royi was passing createRoot() root to unmountComponentAtNode(). So it's new root that I needed to detect inside an old API.
There was a problem hiding this comment.
Inside unmountComponentAtNode, if _reactRootContainer doesn't exist, that's a mistake. I was figuring you would warn regardless, but I suppose you're trying to distinguish between an empty DOM container that was never rendered by anything, and a DOM container that was rendered by createRoot, so you can provide a more helpful message.
Switching from root.render to ReactDOM.render is more interesting, though, I didn't notice you were warning about that one too. That makes sense and I get why you would need the additional expando for that case.
There was a problem hiding this comment.
I suppose you're trying to distinguish between an empty DOM container that was never rendered by anything, and a DOM container that was rendered by createRoot, so you can provide a more helpful message.
Yep 👍
|
I assume #14615 (comment) makes sense but if it doesn't I can follow up. |
* Warn when mixing createRoot() and old APIs * Move container checks to entry points This way further warning check doesn't crash on bad inputs. * Fix Flow * Rename flag to be clearer * managed by -> passed to * Revert accidental change * Fix Fire shim to match
* Warn when mixing createRoot() and old APIs * Move container checks to entry points This way further warning check doesn't crash on bad inputs. * Fix Flow * Rename flag to be clearer * managed by -> passed to * Revert accidental change * Fix Fire shim to match
It's confusing when you mix those — e.g. if you render with
createRoot().render()but then callunmountComponentAtNode(). Sometimes it warns with misleading warnings, and sometimes (in particular, if there's existing content) it didn't warn at all. We don't have guarantees around how it works either (e.g. unmounting doesn't work).I added an explicit DEV-only flag for whether something is a new-style root, and I warn whenever you try to mix the API one way or the other. The message suggests the right API to use. I didn't change the behavior.
I didn't clean up false positive further warnings because the code for them is all over the place and it's hard to untangle the assumptions. So warning at entry point seemed like the easiest thing to do. We want to keep these worlds separate anyway.