Root API should clear non-empty roots before mounting #18730
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f45a8a1:
|
|
Seb and I chatted about this out of band yesterday. He suggested trying a different approach- putting an effect on the This works (in that it fixes the new test I added) but is also a lot broader than my initial approach. Several tests failing. Going to dig in and see if the failures are improvements or road blocks for the approach. This also requires implementing |
|
One test case this breaks is in it('Suspense + hydration in legacy mode with no fallback', () => {
const element = document.createElement('div');
element.innerHTML = '<div>Hello World</div>';
const div = element.firstChild;
const ref = React.createRef();
ReactDOM.hydrate(
<React.Suspense>
<div ref={ref}>Hello World</div>
</React.Suspense>,
element,
);
// Because this didn't have a fallback, it was hydrated as if it's
// not a Suspense boundary.
expect(ref.current).toBe(div);
expect(element.innerHTML).toBe('<div>Hello World</div>');
});In this case, |
|
Can you push the code for the new approach? |
|
Last time we discussed this, I think the conclusion was that maybe it shouldn't clear until commit, at which point we should replace. So that you can leave an HTML shim there that would be replaced. But maybe this isn't that useful. |
|
I guess that's what you're doing. |
That's what this PR is implementing. (And what my local commit that I haven't pushed yet is implementing in a different way, via a new effect type). |
@sebmarkbage Sure. It breaks lots of persistence tests too which I haven't had the chance to dig into yet. |
bvaughn
force-pushed
the
always-clear-non-empty-root-container
branch
3 times, most recently
from
572669a
to
88d6d34
Compare
|
Okay, I think every test is passing now except for the one I mentioned above. Still an open question about React Native and React ART configs. |
Unless you're suggesting I call |
|
I think we can’t do this for Portals because people (FB) are using document.body as the container for many Portals. This is kind of unfortunate for a number of event related things and selective hydration but it’s pretty common. It’s likely that a SSR compatible api for portals would require other semantics anyway since there’s no scope of what body knows are hydratable. So that settles that. |
|
CI failure is expected still until I figure out what to do about that one failing test. Probably going to step away for a bit and go running or something 😄 |
bvaughn
force-pushed
the
always-clear-non-empty-root-container
branch
from
09440f4
to
752d60c
Compare
Legacy render-into-subtree API removes children from a container before rendering into it. The root API did not do this previously, but just left the children around in the document. This commit adds a new FiberRoot flag to clear a container's contents before mounting. This is done during the commit phase, to avoid multiple, observable mutations.
This avoids breaking support for ReactDOM.hydration in legacy mode with no fallback.
|
Okay. Tests are passing. Still an open question about React Native and React ART configs. Think it's ready for review/feedback. |
bvaughn
force-pushed
the
always-clear-non-empty-root-container
branch
from
752d60c
to
f318360
Compare
Details of bundled changes.Comparing: f342a23...f45a8a1 react-test-renderer
react-dom
react-art
react-native-renderer
react-reconciler
Size changes (experimental) |
Details of bundled changes.Comparing: f342a23...f45a8a1 react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
Size changes (stable) |
|
DevTools CI failure is unrelated to this PR. Looks like a transient issue. |
|
Was @sebmarkbage's comment above addressed?
|
Seb's comment doesn't require any action, does it? This change only applies to I think he was just saying that it wouldn't work for portals after all (referring to this earlier comment). |
|
Thanks for clarifying! Does this also cover the case where we mount to a comment node? I know we do it a lot internally, but I wasn't sure if we wanted to keep supporting it in the future. I think @sebmarkbage mentioned that we no longer want to support comment nodes with the new API. I'll approve but maybe we should add a warning/error/test if they're used. I also left some more comments on different ways to improve perf. |
|
I only explicitly check for I don't think there would be anything to do here in the case of a comment node, since comment nodes can't contain children. In other places in the host config file, when we see a comment node, we grab the |
bvaughn
force-pushed
the
always-clear-non-empty-root-container
branch
from
0cc80fa
to
f45a8a1
Compare
Legacy render-into-subtree API removes children from a container before rendering into it:
react/packages/react-dom/src/client/ReactDOMLegacy.js
Lines 113 to 138 in 4b02b66
The
createRootAPI did not do this. Instead it left previous children around in the document forever. This commit brings thecreateRootAPI into alignment withrender- with a small exception. Pre-existing container children don't get cleared until the commit phase.Note that this PR introduces a new DOM host config method,
clearContainer, that may be used by theuseMutableSourcehook as well in the case of hydration (see #18183)