react icon indicating copy to clipboard operation
react copied to clipboard

Bug / Docs / Question: Handling interrupted suspense boundaries

Open jacob-ebey opened this issue 1 year ago • 7 comments

In a world where portions of the component tree can hydrate and become intractable with child components still being suspended, does documentation / guidance exist on how to handle these cases?

Example: https://simple-remix-deferred-demo.fly.dev/

Green: Hydrated Red: Suspended

image

Clicking either of the "Link" elements in the hydrated portion of the page, this will trigger a re-render and causes the suspense boundary to receive an update before it finishes: https://reactjs.org/docs/error-decoder.html/?invariant=421 This isn't an issue in terms of app functionality as implemented as I was under the assumption parent updates from interactive components would be expected, I mean, why wouldn't they?

So I guess there are a few questions I have:

  1. Why does react log this error? Is it an actual error?
  2. Are there implications I can't think of that would cause your average bare bones suspense usage to fall over and that's the reason this message exists?
  3. When you expect updates to happen when boundaries are still suspended, is there a documented way to handle this to avoid this message?

jacob-ebey avatar Jul 19 '22 22:07 jacob-ebey

Is there a sandbox reproducing this? Ideally without a framework.

gaearon avatar Jul 19 '22 23:07 gaearon

I could get you one later. It's simple though:

SSR and suspend. Before suspense boundary resolves, hydrate the parent component and set state.

jacob-ebey avatar Jul 19 '22 23:07 jacob-ebey

Here ya go: https://github.com/jacob-ebey/react-suspense-reproduction

steps:

  • clone
  • npm i
  • npm start
  • load page and click button when boundary is suspended to see message in console

jacob-ebey avatar Jul 20 '22 00:07 jacob-ebey

@gaearon, friendly ping on this, happy to provide more information if needed. This seems like a common case to me and would love to figure out how to handle this so users don't see a scary error message in the terminal.

jacob-ebey avatar Oct 24 '22 22:10 jacob-ebey

Do we need another equivalent of suppressHydrationWarning for <Suspense>?

jacob-ebey avatar Oct 24 '22 23:10 jacob-ebey

For prosperity, there are two ways to suppress the warning that I know of:

  1. Use startTransition: https://github.com/tom-sherman/react-suspense-reproduction/commit/24388fcfa2b2838d511cc9ad509827da1b8e99f2
  2. Memoize the suspense boundary so that the state update doesn't cause it to rerender: https://github.com/tom-sherman/react-suspense-reproduction/commit/ebcdeb6674d8269e48bb370552a683859a3b95bb

tom-sherman avatar Oct 24 '22 23:10 tom-sherman

From #24082

We can go even further and allow updates at higher priority (though not sync) by implementing selective hydration at the root, like we do for Suspense boundaries: interrupt the current render, attempt hydration at slightly higher priority than the update, then continue rendering the update. I haven't implemented this yet, but I've structured the code in anticipation of adding this later.

Could your reproduction be hitting this un-implemented case?

tom-sherman avatar Oct 24 '22 23:10 tom-sherman

I think the question is not about direction per se (the error message already tells you to use startTransition) - but why.

Why does react log this error? Is it an actual error?

Kind of. It's a "recoverable error" which is a new thing in React 18 that we need to document better how to deal with. You can customize display using onRecoverableError. It's not like an error thrown and caught by an error boundary because those are actually user visible error and the normal flow can't be rendered. It's not like console.error in DEV because those usually have a fallback behavior that might be wrong. It's something that happens where we are able to recover by rendering again but it might be subtly different or usually just bad for perf.

There's typically a way to deal with it though. It might not always be worth it. E.g. maybe it only happens if someone does a particular interaction really fast in some edge case. If it's always happening such as when you do initial render or on every navigation - then it's probably a problem you need to fix.

Are there implications I can't think of that would cause your average bare bones suspense usage to fall over and that's the reason this message exists?

We can't commit a sync update partially. On principle. To avoid things like switching a theme on the outside and not the inside and other sorts of consistency bugs. So for sync updates that suspend inside a Suspense boundary, we'll hide the content and show the fallback instead. The intention is that this should basically never happen because those updates should've used startTransition instead. In theory you could hit this code but it would be considered bad UX - but maybe not enough to warrant a warning.

For boundaries that haven't hydrated yet this hiding means that we also delete the original DOM - but tbh we shouldn't really have to and maybe we could fix that.

That said, even then it would be considered bad UX to trigger the fallback. Typically you'd delete the Suspense boundary and render something else (e.g. using proper keys), or use a transition.

When you expect updates to happen when boundaries are still suspended, is there a documented way to handle this to avoid this message?

Those should typically be wrapped in startTransition. Almost everything that isn't like a keystroke updating something very regional should be in a startTransition.

sebmarkbage avatar Oct 28 '22 20:10 sebmarkbage

Take the example code-base and assume everything except these lines are hidden behind framework abstractions:

        <p>
          <button onClick={() => setCount(count + 1)}>Click me</button>
        </p>
        <React.Suspense fallback={<SuspendedFallback />}>
          <SuspendedThing />
        </React.Suspense>

In this case, the underlying "framework abstractions" never actually set state or cause a re-render. There is no re-render caused by anything except "user-code".

Are you saying that anytime a suspense boundary is potentially being rendered at or below a component performing a state update it should be wrapped with startTransition? This is where I'm thrown.

jacob-ebey avatar Oct 28 '22 20:10 jacob-ebey

It's hard to talk about an abstract example because reason something should be wrapped in startTransition depends on the use case. It's not that everything should be wrapped but a lot of things should.

If setCount was a route navigation that updates almost the whole screen, or a refresh of the whole screen then it should.

If it's updating a counter as part of some form field, then maybe it don't need to.

If it's the second case then the error falls into this case:

It might not always be worth it. E.g. maybe it only happens if someone does a particular interaction really fast in some edge case.

Because the way it's meant to be used, it's likely it won't actually happen very much or even be that bad when it happens. If that was the case you probably wouldn't even file an issue because you wouldn't be able to repro it.

When something is consistently triggered, we see that it's either because it's something like a navigation that should be categorically be a transition or it's because of something that's bad for perf anyway like two-pass renderers cased by setState in useEffect.

sebmarkbage avatar Oct 28 '22 20:10 sebmarkbage

Is there a reason we have to recreate the whole DOM for a suspense boundary that re-renders in a suspended state? Seems to me like that should go through the standard reconciliation process by default and not be detrimental to performance / cause this "error" to be logged.

jacob-ebey avatar Oct 29 '22 00:10 jacob-ebey

I forget the reasoning but yea I noted that too. It might be outdated.

tbh we shouldn't really have to and maybe we could fix that.

That code path handles other scenarios too where we might fail to hydrate it, but I think it would make sense in theory to add a special case (more code).

Note that it would still "hide" it by triggering display: none on all the children and then reveal it again once it loads so it's still not great.

sebmarkbage avatar Oct 29 '22 00:10 sebmarkbage

To follow up on this.

First, we're removing the error/warning in https://github.com/facebook/react/pull/25692. So we can close this.

I'll try to clarify what the issue was, based on my understanding (which isn't complete).

Why does react log this error? Is it an actual error?

It's not exactly an error. (And hence we're removing it.) It's more of a hint that something suboptimal for perf has happened. In particular, if I understand correctly, it complains that we've had to throw away server HTML due to a client interaction during hydration. Which as you said is technically valid — but still unfortunate.

I think the original bet with this message was that it's uncommon enough to interact with the page fast enough to hit it (hydration is supposed to be fast). Like, you've had to add a delay to the demo to see it. On the other hand, it's valuable to see this message if you always hit it — e.g. if you do a Redux dispatch during hydration which causes setState or changes an external store. Then you would see it in the console, do some debugging, and solve a deopt.

For example, it's not great if some unnecessary setState call in useEffect always causes your HTML to be thrown away during hydration. It seems like something you'd want to know because it defeats some benefits of SSR. Hence this message.

Are there implications I can't think of that would cause your average bare bones suspense usage to fall over and that's the reason this message exists?

Not sure I understand what you mean by this but I hope my explanation above makes sense. It was supposed to be a way for you to notice that something is updating state (and thus making server HTML potentially stale so it has to be thrown away) during hydration. The bet was that it's unlikely that in most cases it would be due to a user interaction — so it probably means something is setting state in your code. And setting state during hydration isn't great. It doesn't mean you need to prevent setting state during hydration. It's more that you need to think about why you're doing it. State represents user interaction. If there was no user interaction, why is state being set? Nothing "happened".

Of course, that doesn't apply to your example where state actually was being set. I presume the Link was changing the router state. (I'm not sure because I haven't seen the source for the original example.) However, router state updates are supposed to be wrapped in startTransition in React model. E.g. that's how the new Next router works. That's also what the error message says to do. (Note I don't quite remember how transitions interact with hydration so I can't answer why it was "okay" for transitions but not "okay" for regular updates.)

When you expect updates to happen when boundaries are still suspended, is there a documented way to handle this to avoid this message?

It depends on what kind of update it is. Transition updates would not show this message. Sync updates are fired in response to interactions like clicks. The idea with this message was that you're probably not going to click during hydration anyway, so you won't be bothered by the message. But this seems like it might've been a wrong bet. And from what I understand, the follow-up to https://github.com/facebook/react/pull/25692 would even solve some cases for sync updates too. I'm not sure I understand how that part works so I can't comment on that.

I hope this helps. I'm sorry I haven't responded earlier.

gaearon avatar Nov 16 '22 16:11 gaearon

To be fair I think the discussion above is already more insightful than my comment but I wanted to at least try my best to respond to the original post. If you have more questions about this please feel free to ask here.

gaearon avatar Nov 16 '22 16:11 gaearon

State represents user interaction. If there was no user interaction, why is state being set? Nothing "happened".

A common reason for changing the app state before hydration finishes is when some client context, such as dark/light mode, viewport width etc, requires changes to the UI. For example, suppose the app renders a mobile nav in SSR to optimise for mobile delivery, but when hydration starts on the client, the viewport width is large enough to not require the mobile nav anymore and should be discarded. How do you recommend handling such cases?

max-ch9i avatar Apr 04 '23 09:04 max-ch9i

One way to handle this situation is to use CSS media queries to conditionally show or hide the mobile navigation based on the viewport width. This way, the mobile navigation will be automatically hidden when the viewport width is large enough, without requiring any changes to the app state.

Another approach is to use JavaScript to detect the viewport width on the client side and update the app state accordingly. This can be done by adding an event listener for the resize event and updating the app state when the viewport width changes. However, this approach may require additional logic to ensure that the app state is updated correctly during hydration.

It’s important to note that changing the app state before hydration finishes can result in hydration errors if the server-rendered markup does not match the client-rendered markup. To avoid this issue, it’s recommended to delay any changes to the app state until after hydration has completed.

jamesamuli avatar Apr 04 '23 12:04 jamesamuli

For example, suppose the app renders a mobile nav in SSR to optimise for mobile delivery, but when hydration starts on the client, the viewport width is large enough to not require the mobile nav anymore and should be discarded. How do you recommend handling such cases?

Ideally these would not be based on state in the first place. For example, if we're talking about mobile nav or theme — how can you know whether to render mobile or desktop nav on the server? How do you know which theme to render on the server? If you render a mobile version with a light theme, but the user has a desktop window with a dark theme, do you want them to keep seeing the wrong UI right up to the moment the hydration starts? This seems to defeat the purpose of SSR if the initial UI is nowhere close to what ends up being rendered.

One common way to handle this is to avoid relying on things like theme or window size during rendering. Instead, you can use CSS variables and/or media queries (in CSS). In the <head>, you can place a <script> that sets the root CSS variable so that the rest of your CSS switches accordingly. Then the initial HTML would look correct and you wouldn't need to update state either.

gaearon avatar Apr 04 '23 22:04 gaearon