Skip to content

Commit

Permalink
Remove recoverable error when a sync update flows into a dehydrated b…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
sebmarkbage authored and rickhanlonii committed Dec 3, 2022
1 parent 0cf6694 commit 8447b25
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 70 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -453,12 +453,6 @@ describe('ReactDOMServerPartialHydration', () => {

Scheduler.unstable_flushAll();
jest.runAllTimers();
expect(Scheduler).toHaveYielded([
'This Suspense boundary received an update before it finished ' +
'hydrating. This caused the boundary to switch to client rendering. ' +
'The usual way to fix this is to wrap the original update ' +
'in startTransition.',
]);

expect(hydrated.length).toBe(1);
expect(deleted.length).toBe(1);
Expand Down Expand Up @@ -1102,12 +1096,6 @@ describe('ReactDOMServerPartialHydration', () => {
root.render(<App text="Hi" className="hi" />);
Scheduler.unstable_flushAll();
jest.runAllTimers();
expect(Scheduler).toHaveYielded([
'This Suspense boundary received an update before it finished ' +
'hydrating. This caused the boundary to switch to client ' +
'rendering. The usual way to fix this is to wrap the original ' +
'update in startTransition.',
]);

// Flushing now should delete the existing content and show the fallback.

Expand Down Expand Up @@ -1191,12 +1179,6 @@ describe('ReactDOMServerPartialHydration', () => {
// Flushing now should delete the existing content and show the fallback.
Scheduler.unstable_flushAll();
jest.runAllTimers();
expect(Scheduler).toHaveYielded([
'This Suspense boundary received an update before it finished ' +
'hydrating. This caused the boundary to switch to client rendering. ' +
'The usual way to fix this is to wrap the original update ' +
'in startTransition.',
]);

expect(container.getElementsByTagName('span').length).toBe(1);
expect(ref.current).toBe(span);
Expand Down Expand Up @@ -1284,12 +1266,6 @@ describe('ReactDOMServerPartialHydration', () => {
suspend = false;
resolve();
await promise;
expect(Scheduler).toHaveYielded([
'This Suspense boundary received an update before it finished ' +
'hydrating. This caused the boundary to switch to client rendering. ' +
'The usual way to fix this is to wrap the original update ' +
'in startTransition.',
]);

Scheduler.unstable_flushAll();
jest.runAllTimers();
Expand Down Expand Up @@ -1602,12 +1578,6 @@ describe('ReactDOMServerPartialHydration', () => {
// Flushing now should delete the existing content and show the fallback.
Scheduler.unstable_flushAll();
jest.runAllTimers();
expect(Scheduler).toHaveYielded([
'This Suspense boundary received an update before it finished ' +
'hydrating. This caused the boundary to switch to client rendering. ' +
'The usual way to fix this is to wrap the original update ' +
'in startTransition.',
]);

expect(container.getElementsByTagName('span').length).toBe(0);
expect(ref.current).toBe(null);
Expand Down Expand Up @@ -2360,12 +2330,6 @@ describe('ReactDOMServerPartialHydration', () => {
// This will force all expiration times to flush.
Scheduler.unstable_flushAll();
jest.runAllTimers();
expect(Scheduler).toHaveYielded([
'This Suspense boundary received an update before it finished ' +
'hydrating. This caused the boundary to switch to client rendering. ' +
'The usual way to fix this is to wrap the original update ' +
'in startTransition.',
]);

// This will now be a new span because we weren't able to hydrate before
const newSpan = container.getElementsByTagName('span')[0];
Expand Down
26 changes: 9 additions & 17 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -2717,9 +2717,6 @@ function updateDehydratedSuspenseComponent(
current,
workInProgress,
renderLanes,
// TODO: When we delete legacy mode, we should make this error argument
// required — every concurrent mode path that causes hydration to
// de-opt to client rendering should have an error message.
null,
);
}
Expand Down Expand Up @@ -2809,25 +2806,20 @@ function updateDehydratedSuspenseComponent(
}
}

// If we have scheduled higher pri work above, this will probably just abort the render
// since we now have higher priority work, but in case it doesn't, we need to prepare to
// render something, if we time out. Even if that requires us to delete everything and
// skip hydration.
// Delay having to do this as long as the suspense timeout allows us.
// If we have scheduled higher pri work above, this will just abort the render
// since we now have higher priority work. We'll try to infinitely suspend until
// we yield. TODO: We could probably just force yielding earlier instead.
renderDidSuspendDelayIfPossible();
const capturedValue = createCapturedValue(
new Error(
'This Suspense boundary received an update before it finished ' +
'hydrating. This caused the boundary to switch to client rendering. ' +
'The usual way to fix this is to wrap the original update ' +
'in startTransition.',
),
);
// If we rendered synchronously, we won't yield so have to render something.
// This will cause us to delete any existing content.
// TODO: We should ideally have a sync hydration lane that we can apply to do
// a pass where we hydrate this subtree in place using the previous Context and then
// reapply the update afterwards.
return retrySuspenseComponentWithoutHydrating(
current,
workInProgress,
renderLanes,
capturedValue,
null,
);
} else if (isSuspenseInstancePending(suspenseInstance)) {
// This component is still pending more data from the server, so we can't hydrate its
Expand Down
26 changes: 9 additions & 17 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -2717,9 +2717,6 @@ function updateDehydratedSuspenseComponent(
current,
workInProgress,
renderLanes,
// TODO: When we delete legacy mode, we should make this error argument
// required — every concurrent mode path that causes hydration to
// de-opt to client rendering should have an error message.
null,
);
}
Expand Down Expand Up @@ -2809,25 +2806,20 @@ function updateDehydratedSuspenseComponent(
}
}

// If we have scheduled higher pri work above, this will probably just abort the render
// since we now have higher priority work, but in case it doesn't, we need to prepare to
// render something, if we time out. Even if that requires us to delete everything and
// skip hydration.
// Delay having to do this as long as the suspense timeout allows us.
// If we have scheduled higher pri work above, this will just abort the render
// since we now have higher priority work. We'll try to infinitely suspend until
// we yield. TODO: We could probably just force yielding earlier instead.
renderDidSuspendDelayIfPossible();
const capturedValue = createCapturedValue(
new Error(
'This Suspense boundary received an update before it finished ' +
'hydrating. This caused the boundary to switch to client rendering. ' +
'The usual way to fix this is to wrap the original update ' +
'in startTransition.',
),
);
// If we rendered synchronously, we won't yield so have to render something.
// This will cause us to delete any existing content.
// TODO: We should ideally have a sync hydration lane that we can apply to do
// a pass where we hydrate this subtree in place using the previous Context and then
// reapply the update afterwards.
return retrySuspenseComponentWithoutHydrating(
current,
workInProgress,
renderLanes,
capturedValue,
null,
);
} else if (isSuspenseInstancePending(suspenseInstance)) {
// This component is still pending more data from the server, so we can't hydrate its
Expand Down

0 comments on commit 8447b25

Please sign in to comment.