React CM Mode (18) stacking app during hydrateRoot

maraisr commented 3 years ago

React version: 18.0.0-alpha-9f88b5355-20210728

Steps To Reproduce

See repo https://github.com/maraisr/react-suspense-repro

Link to code example:

https://github.com/maraisr/react-suspense-repro

The current behavior

react hydrateRoot is stacking dom trees

will note here that Suspense obviously isnt needed that as nothing is async. I can illustrate here as well if you require a demo with the Suspense needing to do something??

The expected behavior

react hydrateRoot to not stack dom trees

eps1lon commented 3 years ago

Thanks for the feedback.

On the server you're just rendering <App /> (https://github.com/maraisr/react-suspense-repro/blob/main/server.jsx#L24) but on the client you're hydrating a different tree: <Suspense><App /></Suspense> (https://github.com/maraisr/react-suspense-repro/blob/4278aaa9d165a5394e70393f534347210a18c245/client.jsx#L12-L19).

This behavior isn't supported in React 16 as well. However, it is currently silently processed for historic reasons: https://github.com/facebook/react/pull/16943

Though it may make sense to re-introduce a warning in React 18.

maraisr commented 3 years ago

the HTML trees is 100% identical, as Suspense on the server would do nothing? I can see there is a test for it here (https://github.com/facebook/react/blob/main/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js#L417) perhaps the error is just lost?

All im saying above is, the fibre trees arent flushed to the client?? so as far as client is concerned the html is correct? As the net result is, the DOM does in fact equal the fibre tree, and thus should have hydrated?? Or is there anything fancy going on to tell the client about a suspense boundary?

So I understand, youre saying what im seeing in that repro is 100% normal, and 100% okay? And the fix is it put Suspense on the server as well?

eps1lon commented 3 years ago

the HTML trees is 100% identical, as Suspense on the server would do nothing?

HTML is not the important part here. The component trees should be the same.

Suspense on the server does something. It instructs React how it should orchestrate hydration on the client (e.g. prioritazion). https://github.com/reactwg/react-18/discussions/37 goes into more detail about how Suspense works on the server and affects hydration.

So I understand, youre saying what im seeing in that repro is 100% normal, and 100% okay? And the fix is it put Suspense on the server as well?

These are quite loaded terms so I wouldn't use them. I would usually say that the behavior for hydration mismatches is undefined.

Personally, I would prefer a warning but this might be problematic as per https://github.com/facebook/react/pull/16943

But it should be fixed with the same tree on the server (error boundary as well) from what I can tell.

theKashey commented 3 years ago

So the only way to “use” Suspense on the client after 16.10 is to provide a null fallback, ensuring that nothing will suspend during the initial hydration(ie matching server behaviour), allowing such operations only later.

This is working for 16 and 17, but not for 18 or just for CM. @maraisr can you clarify what constrain - react version or rendering mode - is causing double render?

maraisr commented 3 years ago

Suspense on the server does something. It instructs React how it should orchestrate hydration on the client (e.g. prioritazion). reactwg/react-18#37 goes into more detail about how Suspense works on the server and affects hydration.

Ah yeah i see;

<html><body><div id="app"><!--$--><h1>Hello, world!</h1><!--/$--></div><script src="/assets/client.js"></script></body></html>

There is special $ markings in the html for this. So does this mean that in 18, Suspense do occupy html nodes now as well albeit comments?

eps1lon commented 3 years ago

So does this mean that in 18, Suspense do occupy html nodes now as well albeit comments?

I think this is just an implementation detail that you shouldn't need to worry about. As long as you use the appropriate server rendering API and client side API and make sure the trees are the same, everything should work.

maraisr commented 3 years ago

As long as you use the appropriate server rendering API and client side API and make sure the trees are the same, everything should work.

From my repo above, is the apis appropriate? Simply just trees not being the same being the issue? Can close this if that's the case? (Maybe just throw some errors in client??)

eps1lon commented 3 years ago

From my repo above, is the apis appropriate?

Yep 👍🏻

Can close this if that's the case? (Maybe just throw some errors in client??)

I would still like to get some input from a core member whether we still want to silently fail in React 18 or if we should add a dev-only warning similar to how we warn on hydration mismatches for HTML.

maraisr commented 3 years ago

Ya probably not the best thingy to stack dom in any case 😅

sebmarkbage commented 3 years ago

There should be a hydration error in dev mode. Is that not the case? If so that’s a bug.

It’s not a bug that it stacks the DOM node. It’s an unfortunate consequence of us needing some way to inject script tags and other extra nodes that might end up in the root as extra nodes. Currently the plan is to just live with it because it’s such an obvious error and only happens directly at the root so once you fix it, it doesn’t happen again.

eps1lon commented 3 years ago