Bug: Streamed hydration hangs when consecutive Suspense boundaries suspend during streaming

[tannerlinsley]

When streaming from the server, I've encountered a bug where client-side hydration with hydrateRoot() will seemingly "pause" and never complete, leaving html tags with hidden and id attributes hanging around. Any user events cause this error to show, likely because the app still thinks it's in the middle of hydrating. Interestingly enough, if you switch the promise timeouts in routeConfig so that post resolves earlier than posts, then you will not have this issue. So to summarize for this example: if only one boundary suspends, all is well, but if two boundaries suspend, we see this bug.

Hoping this is something stupid on my part, but stuck nonetheless.

React version: 18.2.0

Steps To Reproduce

1. Load the example at https://stackblitz.com/github/tanstack/router/tree/beta/examples/react/basic-ssr?file=src%2FApp.tsx
2. Paste /posts/1 into the preview URL to trigger a server-side load of that full URL
3. Inspect the dom

Link to code example:

https://stackblitz.com/github/tanstack/router/tree/beta/examples/react/basic-ssr?file=src%2FApp.tsx

The current behavior

The markup is not visible (some of it is hidden)

The expected behavior

All of the markup should be visible

[gaearon]

It would be great to have a self-contained example (no third party code) to confirm whether it's a React bug or not.

[hosseinmd]

This worked

export const routeConfig = createRouteConfig().addChildren([
  postRoute,
  indexRoute,
]);

Maybe the issue is related to @tanstack/react-router

[tannerlinsley]

This worked

export const routeConfig = createRouteConfig().addChildren([
  postRoute,
  indexRoute,
]);

Maybe the issue is related to @tanstack/react-router

This only triggers one suspense boundary. So that’s expected.

[tannerlinsley]

It would be great to have a self-contained example (no third party code) to confirm whether it's a React bug or not.

I fully agree. I’ll do my best. But if the issue is with the way I’m implanting suspenseful patterns in the library I would equally want to know why.

[gaearon]

Yea, I just mean that if there's a smaller example then (even if the issue is with the pattern) I'd be able to tell why. In fact just copy pasting the library into the sandbox so that we can start narrowing it down would be helpful. The issue is just that since it's on npm, it's impossible to work by removing pieces and seeing what's essential to the problem.

[tannerlinsley]

I trust me I know the pain. I’ll improve the material as I can.

Tanner Linsley On Nov 19, 2022 at 8:53 AM -0700, dan @.***>, wrote:

Yea, I just mean that if there's a smaller example then (even if the issue is with the pattern) I'd be able to tell why. In fact just copy pasting the library into the sandbox so that we can start narrowing it down would be helpful. The issue is just that since it's on npm, it's impossible to work by removing pieces and seeing what's essential to the problem. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[tannerlinsley]

Got a very watered down example that I'm using to bridge the gaps, so it's still a WIP, but its here: https://stackblitz.com/edit/github-k3gulh?file=src%2FApp.tsx,src%2Fentry-server.tsx,src%2Fstore.tsx,src%2Fentry-client.tsx

Something I noticed: The working example streams a script tag in that looks like this:

function $RC(a,b){a=document.getElementById(a);b=document.getElementById(b);b.parentNode.removeChild(b);if(a){a=a.previousSibling;var f=a.parentNode,c=a.nextSibling,e=0;do{if(c&&8===c.nodeType){var d=c.data;if("/$"===d)if(0===e)break;else e--;else"$"!==d&&"$?"!==d&&"$!"!==d||e++}d=c.nextSibling;f.removeChild(c);c=d}while(c);for(;b.firstChild;)f.insertBefore(b.firstChild,c);a.data="$";a._reactRetry&&a._reactRetry()}};$RC("B:0","S:0")

But I don't see anything similar in my buggy example. Is this a helpful difference?

[gaearon]

Re: https://github.com/facebook/react/issues/25710#issuecomment-1321470216, just to clarify — what is the expected behavior and what is the actual behavior?

[tannerlinsley]

https://github.com/facebook/react/issues/25710#issuecomment-1321470216 Works fine and as expected. So now I’m trying to find the differences between the two.

[gaearon]

Looking again at the original example:

This seems to suggest there's a <Suspense> boundary outside of the <body>. Having <Suspense> outside of <body> is not supported and should error, but it seems like we don't have an error for this. So maybe this is why it gets into a confused state later.

The reason why this is not supported is that there's "nowhere" to render the fallback. E.g. <Suspense fallback={<Spinner />}><html>...</html></Suspense> tells React to render the spinner outside <html> which doesn't make sense. So we should error for this on the server.

Slightly tangential, but a Suspense boundary at the very root is not recommended. The user needs to be able to pick where the shell of the app "ends". The shell is defined as the place outside all Suspense boundaries. It is important for the user to be able to choose what UI goes there — e.g. to tell React to always emit a skeleton of head+main+sidebar+footer as a minimal page instead of a spinner around them.

(I don't know if this is actually the cause btw — but if there's a sandbox with the router as one of the files so I can edit its code, I can check that).

[tannerlinsley]

You might be on to something. I do in fact have a suspense boundary around the <html> of the app. I had no idea this isn't supported.

If this does turn out to be the issue, I will inevitably ask why we can't support it. Seems like a valid use case.

[Andarist]

If this does turn out to be the issue, I will inevitably ask why we can't support it. Seems like a valid use case.

I think this was already answered by Dan:

The reason why this is not supported is that there's "nowhere" to render the fallback. E.g. <Suspense fallback={<Spinner />}><html>...</html></Suspense> tells React to render the spinner outside <html> which doesn't make sense. So we should error for this on the server.

[gaearon]

To clarify, to the best of my knowledge, suspending in the shell is supported, including outside the body. This will hold back the shell until the data resolves. (If you're hitting bugs, it's worth checking whether react@next + react-dom@next release channel fixes them.) It's specifically <Suspense> node outside <body> that's not supported — both for the reason I mentioned earlier and because it defeats the point. The point is that React should wait for the minimal shell to complete before starting the stream. Not emit the fallback immediately.

[tannerlinsley]

This makes way more sense. In hindsight, it’s obvious now that you weren’t referring to suspending in general. Phew!! 😅 Y’all had me scared for a moment that I couldn’t suspend in the shell!

[tannerlinsley]

I’ll see what I can do.