Bug / Docs / Question: Handling interrupted suspense boundaries #24959
Comments
jacob-ebey
added
the
Status: Unconfirmed
|
Is there a sandbox reproducing this? Ideally without a framework. |
|
I could get you one later. It's simple though: SSR and suspend. Before suspense boundary resolves, hydrate the parent component and set state. |
|
Here ya go: https://github.com/jacob-ebey/react-suspense-reproduction steps:
|
|
@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
changed the title
|
Do we need another equivalent of |
|
For prosperity, there are two ways to suppress the warning that I know of:
|
|
From #24082
Could your reproduction be hitting this un-implemented case? |
|
I think the question is not about direction per se (the error message already tells you to use startTransition) - but why.
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 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.
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.
Those should typically be wrapped in startTransition. Almost everything that isn't like a keystroke updating something very regional should be in a startTransition. |
|
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 |
|
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:
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. |
|
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. |
|
I forget the reasoning but yea I noted that too. It might be outdated.
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 |
…oundary (#25692) This just removes the error but the underlying issue is still there, and it's likely that the best course of action is to not update in effects and to wrap most updates in startTransition. However, that's more of a performance concern which is not something we generally do even in recoverable errors since they're less actionable and likely belong in another channel. It is also likely that in many cases this happens so rarely because you have to interact quickly enough that it can often be ignored. After changes to other parts of the model, this only happens for sync/discrete updates. There are three scenarios that can happen: - We replace a server rendered fallback with a client rendered fallback. Other than this potentially causing some flickering in the loading state, it's not a big deal. - We replace the server rendered content with a client side fallback if this suspends on the client. This is in line with what would happen anyway. We will loose state of forms which is not intended semantics. State and animations etc would've been lost anyway if it was client-side so that's not a concern. - We replace the server rendered content with a client side rendered tree and lose selection/state and form state. While the content looks the same, which is unfortunate. In most scenarios it's a bad loading state but it's the same scenario as flushing sync client-side. So it's not so bad. The big change here is that we consider this a bug of React that we should fix. Therefore it's not actionable to users today because it should just get fixed. So we're removing the error early. Although anyone that has fixed these issues already are probably better off for it. To fix this while still hydrating we need to be able to rewind a sync tree and then replay it. @tyao1 is going to add a Sync hydration lane. This is will allow us to rewind the tree when we hit this state, and replay it given the previous Context, hydrate and then reapply the update. The reason we didn't do this originally is because it causes sync mode to unwind where as for backwards compatibility we didn't want to cause that breaking semantic - outside Suspense boundaries - and we don't want that semantic longer term. We're only do this as a short term fix. We should also have a way to leave a partial tree in place. If the sync hydration lane suspends, we should be able to switch to a client side fallback without throwing away the state of the DOM and then hydrate later. We now know how we want to fix this longer term. We're going to move all Contexts into resumable trees like what Fizz/Flight does. That way we can leave the original Context at the hydration boundaries and then resume from there. That way the rewinding would never happen even in the existence of a sync hydration lane which would only apply locally to the dehydrated tree. So the steps are 1) remove the error 2) add the sync hydration lane with rewinding 3) Allow hiding server-rendered content while still not hydrated 4) add resumable contexts at these boundaries. Fixes #25625 and #24959.
|
To follow up on this. First, we're removing the error/warning in #25692. So we can close this. I'll try to clarify what the issue was, based on my understanding (which isn't complete).
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 For example, it's not great if some unnecessary
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
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 #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. |
|
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. |
…oundary (facebook#25692) This just removes the error but the underlying issue is still there, and it's likely that the best course of action is to not update in effects and to wrap most updates in startTransition. However, that's more of a performance concern which is not something we generally do even in recoverable errors since they're less actionable and likely belong in another channel. It is also likely that in many cases this happens so rarely because you have to interact quickly enough that it can often be ignored. After changes to other parts of the model, this only happens for sync/discrete updates. There are three scenarios that can happen: - We replace a server rendered fallback with a client rendered fallback. Other than this potentially causing some flickering in the loading state, it's not a big deal. - We replace the server rendered content with a client side fallback if this suspends on the client. This is in line with what would happen anyway. We will loose state of forms which is not intended semantics. State and animations etc would've been lost anyway if it was client-side so that's not a concern. - We replace the server rendered content with a client side rendered tree and lose selection/state and form state. While the content looks the same, which is unfortunate. In most scenarios it's a bad loading state but it's the same scenario as flushing sync client-side. So it's not so bad. The big change here is that we consider this a bug of React that we should fix. Therefore it's not actionable to users today because it should just get fixed. So we're removing the error early. Although anyone that has fixed these issues already are probably better off for it. To fix this while still hydrating we need to be able to rewind a sync tree and then replay it. @tyao1 is going to add a Sync hydration lane. This is will allow us to rewind the tree when we hit this state, and replay it given the previous Context, hydrate and then reapply the update. The reason we didn't do this originally is because it causes sync mode to unwind where as for backwards compatibility we didn't want to cause that breaking semantic - outside Suspense boundaries - and we don't want that semantic longer term. We're only do this as a short term fix. We should also have a way to leave a partial tree in place. If the sync hydration lane suspends, we should be able to switch to a client side fallback without throwing away the state of the DOM and then hydrate later. We now know how we want to fix this longer term. We're going to move all Contexts into resumable trees like what Fizz/Flight does. That way we can leave the original Context at the hydration boundaries and then resume from there. That way the rewinding would never happen even in the existence of a sync hydration lane which would only apply locally to the dehydrated tree. So the steps are 1) remove the error 2) add the sync hydration lane with rewinding 3) Allow hiding server-rendered content while still not hydrated 4) add resumable contexts at these boundaries. Fixes facebook#25625 and facebook#24959.
…oundary (#25692) This just removes the error but the underlying issue is still there, and it's likely that the best course of action is to not update in effects and to wrap most updates in startTransition. However, that's more of a performance concern which is not something we generally do even in recoverable errors since they're less actionable and likely belong in another channel. It is also likely that in many cases this happens so rarely because you have to interact quickly enough that it can often be ignored. After changes to other parts of the model, this only happens for sync/discrete updates. There are three scenarios that can happen: - We replace a server rendered fallback with a client rendered fallback. Other than this potentially causing some flickering in the loading state, it's not a big deal. - We replace the server rendered content with a client side fallback if this suspends on the client. This is in line with what would happen anyway. We will loose state of forms which is not intended semantics. State and animations etc would've been lost anyway if it was client-side so that's not a concern. - We replace the server rendered content with a client side rendered tree and lose selection/state and form state. While the content looks the same, which is unfortunate. In most scenarios it's a bad loading state but it's the same scenario as flushing sync client-side. So it's not so bad. The big change here is that we consider this a bug of React that we should fix. Therefore it's not actionable to users today because it should just get fixed. So we're removing the error early. Although anyone that has fixed these issues already are probably better off for it. To fix this while still hydrating we need to be able to rewind a sync tree and then replay it. @tyao1 is going to add a Sync hydration lane. This is will allow us to rewind the tree when we hit this state, and replay it given the previous Context, hydrate and then reapply the update. The reason we didn't do this originally is because it causes sync mode to unwind where as for backwards compatibility we didn't want to cause that breaking semantic - outside Suspense boundaries - and we don't want that semantic longer term. We're only do this as a short term fix. We should also have a way to leave a partial tree in place. If the sync hydration lane suspends, we should be able to switch to a client side fallback without throwing away the state of the DOM and then hydrate later. We now know how we want to fix this longer term. We're going to move all Contexts into resumable trees like what Fizz/Flight does. That way we can leave the original Context at the hydration boundaries and then resume from there. That way the rewinding would never happen even in the existence of a sync hydration lane which would only apply locally to the dehydrated tree. So the steps are 1) remove the error 2) add the sync hydration lane with rewinding 3) Allow hiding server-rendered content while still not hydrated 4) add resumable contexts at these boundaries. Fixes #25625 and #24959.
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? |
|
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. |
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 |
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
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:
The text was updated successfully, but these errors were encountered: