React CM Mode (18) stacking app during hydrateRoot #21985

maraisr · 2021-07-29T04:52:59Z

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

The text was updated successfully, but these errors were encountered:

eps1lon · 2021-07-29T05:48:35Z

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: #16943

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

maraisr · 2021-07-29T05:54:53Z

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 · 2021-07-29T06:21:45Z

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). reactwg/react-18#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 #16943

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

theKashey · 2021-07-29T06:28:01Z

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 · 2021-07-29T06:34:30Z

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 · 2021-07-29T06:51:00Z

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 · 2021-07-29T06:58:24Z

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 · 2021-07-29T07:59:15Z

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 · 2021-07-29T08:53:07Z

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

sebmarkbage · 2021-07-29T13:15:22Z

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 · 2021-07-29T14:26:12Z

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

Seeing no error message when using hydrateRoot: https://codesandbox.io/s/jovial-cherry-2qxn3?file=/src/index.js

Looks like

react/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js

Lines 442 to 460 in 34308b5

    
           it('Suspense + hydration in legacy mode (at root)', () => { 
        
             const element = document.createElement('div'); 
        
             element.innerHTML = '<div>Hello World</div>'; 
        
             const div = element.firstChild; 
        
             const ref = React.createRef(); 
        
             ReactDOM.hydrate( 
        
               <React.Suspense fallback={null}> 
        
                 <div ref={ref}>Hello World</div> 
        
               </React.Suspense>, 
        
               element, 
        
             ); 
        
             // The content should've been client rendered. 
        
             expect(ref.current).not.toBe(div); 
        
             // Unfortunately, since we don't delete the tail at the root, a duplicate will remain. 
        
             expect(element.innerHTML).toBe( 
        
               '<div>Hello World</div><div>Hello World</div>', 
        
             ); 
        
           });

is already covering the current behavior but does not include assertions about console errors.

maraisr · 2021-07-29T21:19:30Z

Is that not the case? If so that’s a bug

Yup there's no error or messaging in either mode.

I guess that's why I originally thought there's a bug with React. But guess the bug in the end is just no messaging 😅

Thanks for giving this issue the time of day tho folk ❤️

gaearon · 2022-03-25T23:16:39Z

There is an error message in 18 RC.
https://codesandbox.io/s/eloquent-cherry-iwt1zz?file=/src/index.js

Though it's not super descriptive about where the error happens.

maraisr added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jul 29, 2021

maraisr mentioned this issue Jul 29, 2021

React 16.10 broke Next.js/SSR applications #16938

Closed

eps1lon added React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React CM Mode (18) stacking app during hydrateRoot #21985

React CM Mode (18) stacking app during hydrateRoot #21985

maraisr commented Jul 29, 2021 •

edited

eps1lon commented Jul 29, 2021

maraisr commented Jul 29, 2021 •

edited

eps1lon commented Jul 29, 2021

theKashey commented Jul 29, 2021

maraisr commented Jul 29, 2021

eps1lon commented Jul 29, 2021

maraisr commented Jul 29, 2021

eps1lon commented Jul 29, 2021

maraisr commented Jul 29, 2021

sebmarkbage commented Jul 29, 2021

eps1lon commented Jul 29, 2021 •

edited

maraisr commented Jul 29, 2021

gaearon commented Mar 25, 2022

React CM Mode (18) stacking app during hydrateRoot #21985

React CM Mode (18) stacking app during hydrateRoot #21985

Comments

maraisr commented Jul 29, 2021 • edited

Steps To Reproduce

The current behavior

The expected behavior

eps1lon commented Jul 29, 2021

maraisr commented Jul 29, 2021 • edited

eps1lon commented Jul 29, 2021

theKashey commented Jul 29, 2021

maraisr commented Jul 29, 2021

eps1lon commented Jul 29, 2021

maraisr commented Jul 29, 2021

eps1lon commented Jul 29, 2021

maraisr commented Jul 29, 2021

sebmarkbage commented Jul 29, 2021

eps1lon commented Jul 29, 2021 • edited

maraisr commented Jul 29, 2021

gaearon commented Mar 25, 2022

maraisr commented Jul 29, 2021 •

edited

maraisr commented Jul 29, 2021 •

edited

eps1lon commented Jul 29, 2021 •

edited