From c09b4a07434ac45363fd5976bf639a00e3e805f9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 17 Oct 2022 00:31:28 -0400 Subject: [PATCH] Bugfix: Suspending in shell during discrete update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes a bug that happens when you suspend in the shell (the part of the tree that is not wrapped in a Suspense boundary) during a discrete update. There were two underyling issues. One was just a mistake: RootDidNotComplete needs to be handled in both renderRootConcurrent and renderRootSync, but it was only handled in renderRootConcurrent. I did it this way because I thought this path was unreachable during a sync update, but I neglected to consider that renderRootSync is sometimes called for non-concurrent lanes, like when recovering from an error, or patching up a mutation to an external store. After I fixed that oversight, the other issue is that we intentionally error if the shell suspends during a sync update. The idea was that you should either wrap the tree in a Suspense boundary, or you should mark the update as a transition to allow React to suspend. However, this did not take into account selective hydration, which can force a sync render before anything has even committed. There's no way in that case to wrap the update in startTransition. Our solution for now is to remove the error that happens when you suspend in the shell during a sync update — even for discrete updates. We will likely revisit this in the future. One appealing possibility is to commit the whole root in an inert state, as if it were a hidden Offscreen tree. Co-authored-by: Sebastian Markbåge --- ...MServerSelectiveHydration-test.internal.js | 77 +++++++++++++++++++ .../src/ReactFiberThrow.new.js | 38 ++++----- .../src/ReactFiberThrow.old.js | 38 ++++----- .../src/ReactFiberWorkLoop.new.js | 14 ++-- .../src/ReactFiberWorkLoop.old.js | 14 ++-- .../ReactSuspenseWithNoopRenderer-test.js | 16 ++-- 6 files changed, 138 insertions(+), 59 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js index 91321ea6dd5bf..048d6c9db2972 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js @@ -1695,4 +1695,81 @@ describe('ReactDOMServerSelectiveHydration', () => { expect(triggeredParent).toBe(false); expect(triggeredChild).toBe(false); }); + + it('can attempt sync hydration if suspended root is still concurrently rendering', async () => { + let suspend = false; + let resolve; + const promise = new Promise(resolvePromise => (resolve = resolvePromise)); + function Child({text}) { + if (suspend) { + throw promise; + } + Scheduler.unstable_yieldValue(text); + return ( + { + e.preventDefault(); + Scheduler.unstable_yieldValue('Clicked ' + text); + }}> + {text} + + ); + } + + function App() { + Scheduler.unstable_yieldValue('App'); + return ( +
+ +
+ ); + } + + const finalHTML = ReactDOMServer.renderToString(); + + expect(Scheduler).toHaveYielded(['App', 'A']); + + const container = document.createElement('div'); + // We need this to be in the document since we'll dispatch events on it. + document.body.appendChild(container); + + container.innerHTML = finalHTML; + + const span = container.getElementsByTagName('span')[0]; + + // We suspend on the client. + suspend = true; + + React.startTransition(() => { + ReactDOMClient.hydrateRoot(container, ); + }); + expect(Scheduler).toFlushAndYieldThrough(['App']); + + // This should attempt to synchronously hydrate the root, then pause + // because it still suspended + const result = dispatchClickEvent(span); + expect(Scheduler).toHaveYielded(['App']); + // The event should not have been cancelled because we didn't hydrate. + expect(result).toBe(true); + + // Finish loading the data + await act(async () => { + suspend = false; + await resolve(); + }); + + // The app should have successfully hydrated and rendered + if ( + gate( + flags => + flags.enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay, + ) + ) { + expect(Scheduler).toHaveYielded(['App', 'A']); + } else { + expect(Scheduler).toHaveYielded(['App', 'A', 'Clicked A']); + } + + document.body.removeChild(container); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index 97e02cd8dec6b..e05b037d9f2c8 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -69,13 +69,13 @@ import { includesSomeLane, mergeLanes, pickArbitraryLane, - includesSyncLane, } from './ReactFiberLane.new'; import { getIsHydrating, markDidThrowWhileHydratingDEV, queueHydrationError, } from './ReactFiberHydrationContext.new'; +import {ConcurrentRoot} from './ReactRootTags'; function createRootErrorUpdate( fiber: Fiber, @@ -421,32 +421,26 @@ function throwException( // No boundary was found. Unless this is a sync update, this is OK. // We can suspend and wait for more data to arrive. - if (!includesSyncLane(rootRenderLanes)) { - // This is not a sync update. Suspend. Since we're not activating a - // Suspense boundary, this will unwind all the way to the root without - // performing a second pass to render a fallback. (This is arguably how - // refresh transitions should work, too, since we're not going to commit - // the fallbacks anyway.) + if (root.tag === ConcurrentRoot) { + // In a concurrent root, suspending without a Suspense boundary is + // allowed. It will suspend indefinitely without committing. // - // This case also applies to initial hydration. + // TODO: Should we have different behavior for discrete updates? What + // about flushSync? Maybe it should put the tree into an inert state, + // and potentially log a warning. Revisit this for a future release. attachPingListener(root, wakeable, rootRenderLanes); renderDidSuspendDelayIfPossible(); return; + } else { + // In a legacy root, suspending without a boundary is always an error. + const uncaughtSuspenseError = new Error( + 'A component suspended while responding to synchronous input. This ' + + 'will cause the UI to be replaced with a loading indicator. To ' + + 'fix, updates that suspend should be wrapped ' + + 'with startTransition.', + ); + value = uncaughtSuspenseError; } - - // This is a sync/discrete update. We treat this case like an error - // because discrete renders are expected to produce a complete tree - // synchronously to maintain consistency with external state. - const uncaughtSuspenseError = new Error( - 'A component suspended while responding to synchronous input. This ' + - 'will cause the UI to be replaced with a loading indicator. To ' + - 'fix, updates that suspend should be wrapped ' + - 'with startTransition.', - ); - - // If we're outside a transition, fall through to the regular error path. - // The error will be caught by the nearest suspense boundary. - value = uncaughtSuspenseError; } } else { // This is a regular error, not a Suspense wakeable. diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index f20047c8f05d1..fcd35f8b32b3d 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -69,13 +69,13 @@ import { includesSomeLane, mergeLanes, pickArbitraryLane, - includesSyncLane, } from './ReactFiberLane.old'; import { getIsHydrating, markDidThrowWhileHydratingDEV, queueHydrationError, } from './ReactFiberHydrationContext.old'; +import {ConcurrentRoot} from './ReactRootTags'; function createRootErrorUpdate( fiber: Fiber, @@ -421,32 +421,26 @@ function throwException( // No boundary was found. Unless this is a sync update, this is OK. // We can suspend and wait for more data to arrive. - if (!includesSyncLane(rootRenderLanes)) { - // This is not a sync update. Suspend. Since we're not activating a - // Suspense boundary, this will unwind all the way to the root without - // performing a second pass to render a fallback. (This is arguably how - // refresh transitions should work, too, since we're not going to commit - // the fallbacks anyway.) + if (root.tag === ConcurrentRoot) { + // In a concurrent root, suspending without a Suspense boundary is + // allowed. It will suspend indefinitely without committing. // - // This case also applies to initial hydration. + // TODO: Should we have different behavior for discrete updates? What + // about flushSync? Maybe it should put the tree into an inert state, + // and potentially log a warning. Revisit this for a future release. attachPingListener(root, wakeable, rootRenderLanes); renderDidSuspendDelayIfPossible(); return; + } else { + // In a legacy root, suspending without a boundary is always an error. + const uncaughtSuspenseError = new Error( + 'A component suspended while responding to synchronous input. This ' + + 'will cause the UI to be replaced with a loading indicator. To ' + + 'fix, updates that suspend should be wrapped ' + + 'with startTransition.', + ); + value = uncaughtSuspenseError; } - - // This is a sync/discrete update. We treat this case like an error - // because discrete renders are expected to produce a complete tree - // synchronously to maintain consistency with external state. - const uncaughtSuspenseError = new Error( - 'A component suspended while responding to synchronous input. This ' + - 'will cause the UI to be replaced with a loading indicator. To ' + - 'fix, updates that suspend should be wrapped ' + - 'with startTransition.', - ); - - // If we're outside a transition, fall through to the regular error path. - // The error will be caught by the nearest suspense boundary. - value = uncaughtSuspenseError; } } else { // This is a regular error, not a Suspense wakeable. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 38994cdef5291..172b956d8fe7f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1063,10 +1063,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) { // The render unwound without completing the tree. This happens in special // cases where need to exit the current render without producing a // consistent tree or committing. - // - // This should only happen during a concurrent render, not a discrete or - // synchronous update. We should have already checked for this when we - // unwound the stack. markRootSuspended(root, lanes); } else { // The render completed. @@ -1111,6 +1107,9 @@ function performConcurrentWorkOnRoot(root, didTimeout) { ensureRootIsScheduled(root, now()); throw fatalError; } + + // FIXME: Need to check for RootDidNotComplete again. The factoring here + // isn't ideal. } // We now have a consistent tree. The next step is either to commit it, @@ -1473,7 +1472,12 @@ function performSyncWorkOnRoot(root) { } if (exitStatus === RootDidNotComplete) { - throw new Error('Root did not complete. This is a bug in React.'); + // The render unwound without completing the tree. This happens in special + // cases where need to exit the current render without producing a + // consistent tree or committing. + markRootSuspended(root, lanes); + ensureRootIsScheduled(root, now()); + return null; } // We now have a consistent tree. Because this is a sync render, we diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 1dacb1fd5685e..a439b90e40843 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1063,10 +1063,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) { // The render unwound without completing the tree. This happens in special // cases where need to exit the current render without producing a // consistent tree or committing. - // - // This should only happen during a concurrent render, not a discrete or - // synchronous update. We should have already checked for this when we - // unwound the stack. markRootSuspended(root, lanes); } else { // The render completed. @@ -1111,6 +1107,9 @@ function performConcurrentWorkOnRoot(root, didTimeout) { ensureRootIsScheduled(root, now()); throw fatalError; } + + // FIXME: Need to check for RootDidNotComplete again. The factoring here + // isn't ideal. } // We now have a consistent tree. The next step is either to commit it, @@ -1473,7 +1472,12 @@ function performSyncWorkOnRoot(root) { } if (exitStatus === RootDidNotComplete) { - throw new Error('Root did not complete. This is a bug in React.'); + // The render unwound without completing the tree. This happens in special + // cases where need to exit the current render without producing a + // consistent tree or committing. + markRootSuspended(root, lanes); + ensureRootIsScheduled(root, now()); + return null; } // We now have a consistent tree. Because this is a sync render, we diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 07838b223e687..a0a7089157440 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -1012,15 +1012,21 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // @gate enableCache - it('errors when an update suspends without a placeholder during a sync update', () => { - // This is an error because sync/discrete updates are expected to produce - // a complete tree immediately to maintain consistency with external state - // — we can't delay the commit. + it('in concurrent mode, does not error when an update suspends without a Suspense boundary during a sync update', () => { + // NOTE: We may change this to be a warning in the future. expect(() => { ReactNoop.flushSync(() => { ReactNoop.render(); }); - }).toThrow('A component suspended while responding to synchronous input.'); + }).not.toThrow(); + }); + + // @gate enableCache + it('in legacy mode, errors when an update suspends without a Suspense boundary during a sync update', () => { + const root = ReactNoop.createLegacyRoot(); + expect(() => root.render()).toThrow( + 'A component suspended while responding to synchronous input.', + ); }); // @gate enableCache