From 15070a9d5b728a39b2d8e498d432ad2aba4980e5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 13 Mar 2023 21:37:46 -0400 Subject: [PATCH] Don't prerender siblings of suspended component MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Today if something suspends, React will continue rendering the siblings of that component. Our original rationale for prerendering the siblings of a suspended component was to initiate any lazy fetches that they might contain. This was when we were more bullish about lazy fetching being a good idea some of the time (when combined with prefetching), as opposed to our latest thinking, which is that it's almost always a bad idea. Another rationale for the original behavior was that the render was I/O bound, anyway, so we might as do some extra work in the meantime. But this was before we had the concept of instant loading states: when navigating to a new screen, it's better to show a loading state as soon as you can (often a skeleton UI), rather than delay the transition. (There are still cases where we block the render, when a suitable loading state is not available; it's just not _all_ cases where something suspends.) So the biggest issue with our existing implementation is that the prerendering of the siblings happens within the same render pass as the one that suspended — _before_ the loading state appears. What we should do instead is immediately unwind the stack as soon as something suspends, to unblock the loading state. If we want to preserve the ability to prerender the siblings, what we could do is schedule special render pass immediately after the fallback is displayed. This is likely what we'll do in the future. However, in the new implementation of `use`, there's another reason we don't prerender siblings: so we can preserve the state of the stack when something suspends, and resume where we left of when the promise resolves without replaying the parents. The only way to do this currently is to suspend the entire work loop. Fiber does not currently support rendering multiple siblings in "parallel". Once you move onto the next sibling, the stack of the previous sibling is discarded and cannot be restored. We do plan to implement this feature, but it will require a not-insignificant refactor. Given that lazy data fetching is already bad for performance, the best trade off for now seems to be to disable prerendering of siblings. This gives us the best performance characteristics when you're following best practices (i.e. hoist data fetches to Server Components or route loaders), at the expense of an already bad pattern a bit worse. Later, when we implement resumable context stacks, we can reenable sibling prerendering. Though even then the use case will mostly be to prerender the CPU-bound work, not lazy fetches. --- .../__tests__/ReactInternalTestUtils-test.js | 4 - .../__tests__/ReactCacheOld-test.internal.js | 39 ++-- .../src/__tests__/ReactDOMFizzServer-test.js | 217 +++++++++--------- ...actDOMFizzSuppressHydrationWarning-test.js | 1 - .../__tests__/ReactDOMHydrationDiff-test.js | 64 +++--- ...DOMServerPartialHydration-test.internal.js | 10 +- .../ReactErrorBoundaries-test.internal.js | 5 +- ...eactLegacyErrorBoundaries-test.internal.js | 5 +- .../src/ReactFiberCompleteWork.js | 6 +- .../src/ReactFiberWorkLoop.js | 35 ++- .../__tests__/ReactBatching-test.internal.js | 2 +- .../src/__tests__/ReactCache-test.js | 2 +- .../ReactConcurrentErrorRecovery-test.js | 30 +-- ...asedOnReactExpirationTime-test.internal.js | 2 +- .../src/__tests__/ReactExpiration-test.js | 2 +- ...tIncrementalErrorHandling-test.internal.js | 76 +++--- .../src/__tests__/ReactLazy-test.internal.js | 24 +- .../__tests__/ReactSuspense-test.internal.js | 24 +- .../__tests__/ReactSuspenseCallback-test.js | 2 +- .../ReactSuspenseEffectsSemantics-test.js | 65 +----- .../ReactSuspensePlaceholder-test.internal.js | 49 ++-- .../ReactSuspenseWithNoopRenderer-test.js | 42 ++-- .../src/__tests__/ReactTransition-test.js | 1 - .../__tests__/ReactTransitionTracing-test.js | 16 -- .../src/__tests__/ReactUse-test.js | 26 +-- .../src/__tests__/useEffectEvent-test.js | 5 +- 26 files changed, 319 insertions(+), 435 deletions(-) diff --git a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js index 0b9cc1f252f79..125cc261de90b 100644 --- a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js +++ b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js @@ -99,13 +99,9 @@ describe('ReactInternalTestUtils', () => { assertLog([ 'A', 'B', - 'C', - 'D', // React will try one more time before giving up. 'A', 'B', - 'C', - 'D', ]); }); diff --git a/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js b/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js index aa25cacc26fc9..117c0b488b178 100644 --- a/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js +++ b/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js @@ -208,19 +208,19 @@ describe('ReactCache', () => { unstable_isConcurrent: true, }, ); - await waitForAll([ - 'Suspend! [1]', - 'Suspend! [2]', - 'Suspend! [3]', - 'Loading...', - ]); + await waitForAll(['Suspend! [1]', 'Loading...']); jest.advanceTimersByTime(100); - assertLog([ - 'Promise resolved [1]', - 'Promise resolved [2]', - 'Promise resolved [3]', - ]); + assertLog(['Promise resolved [1]']); + await waitForAll([1, 'Suspend! [2]']); + + jest.advanceTimersByTime(100); + assertLog(['Promise resolved [2]']); + await waitForAll([1, 2, 'Suspend! [3]']); + + jest.advanceTimersByTime(100); + assertLog(['Promise resolved [3]']); await waitForAll([1, 2, 3]); + expect(root).toMatchRenderedOutput('123'); // Render 1, 4, 5 @@ -232,10 +232,16 @@ describe('ReactCache', () => { , ); - await waitForAll([1, 'Suspend! [4]', 'Suspend! [5]', 'Loading...']); + await waitForAll([1, 'Suspend! [4]', 'Loading...']); + + jest.advanceTimersByTime(100); + assertLog(['Promise resolved [4]']); + await waitForAll([1, 4, 'Suspend! [5]', 'Loading...']); + jest.advanceTimersByTime(100); - assertLog(['Promise resolved [4]', 'Promise resolved [5]']); + assertLog(['Promise resolved [5]']); await waitForAll([1, 4, 5]); + expect(root).toMatchRenderedOutput('145'); // We've now rendered values 1, 2, 3, 4, 5, over our limit of 3. The least @@ -254,11 +260,14 @@ describe('ReactCache', () => { 1, // 2 and 3 suspend because they were evicted from the cache 'Suspend! [2]', - 'Suspend! [3]', 'Loading...', ]); jest.advanceTimersByTime(100); - assertLog(['Promise resolved [2]', 'Promise resolved [3]']); + assertLog(['Promise resolved [2]']); + await waitForAll([1, 2, 'Suspend! [3]', 'Loading...']); + + jest.advanceTimersByTime(100); + assertLog(['Promise resolved [3]']); await waitForAll([1, 2, 3]); expect(root).toMatchRenderedOutput('123'); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index ecb437f72df1f..21ff0d1949e78 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -2949,9 +2949,6 @@ describe('ReactDOMFizzServer', () => { // blocking the main thread. shouldThrow = false; await waitForAll([ - // Finish initial render attempt - 'B', - // Render again, synchronously 'A', 'B', @@ -3902,32 +3899,32 @@ describe('ReactDOMFizzServer', () => { it('supresses hydration warnings when an error occurs within a Suspense boundary', async () => { let isClient = false; - let shouldThrow = true; - function ThrowUntilOnClient({children}) { - if (isClient && shouldThrow) { - throw new Error('uh oh'); - } + function ThrowWhenHydrating({children}) { + // This is a trick to only throw if we're hydrating, because + // useSyncExternalStore calls getServerSnapshot instead of the regular + // getSnapshot in that case. + useSyncExternalStore( + () => {}, + t => t, + () => { + if (isClient) { + throw new Error('uh oh'); + } + }, + ); return children; } - function StopThrowingOnClient() { - if (isClient) { - shouldThrow = false; - } - return null; - } - const App = () => { return (
Loading...}> - +

one

-
+

two

{isClient ? 'five' : 'three'}

-
); @@ -3955,8 +3952,6 @@ describe('ReactDOMFizzServer', () => { }); await waitForAll([ 'Logged recoverable error: uh oh', - 'Logged recoverable error: Hydration failed because the initial UI does not match what was rendered on the server.', - 'Logged recoverable error: Hydration failed because the initial UI does not match what was rendered on the server.', 'Logged recoverable error: There was an error while hydrating this Suspense boundary. Switched to client rendering.', ]); @@ -3986,37 +3981,37 @@ describe('ReactDOMFizzServer', () => { mockError(...args.map(normalizeCodeLocInfo)); }; let isClient = false; - let shouldThrow = true; - function ThrowUntilOnClient({children, message}) { - if (isClient && shouldThrow) { - Scheduler.log('throwing: ' + message); - throw new Error(message); - } + function ThrowWhenHydrating({children, message}) { + // This is a trick to only throw if we're hydrating, because + // useSyncExternalStore calls getServerSnapshot instead of the regular + // getSnapshot in that case. + useSyncExternalStore( + () => {}, + t => t, + () => { + if (isClient) { + Scheduler.log('throwing: ' + message); + throw new Error(message); + } + }, + ); return children; } - function StopThrowingOnClient() { - if (isClient) { - shouldThrow = false; - } - return null; - } - const App = () => { return (
Loading...}> - +

one

-
- + +

two

-
- + +

three

-
- +
); @@ -4047,14 +4042,10 @@ describe('ReactDOMFizzServer', () => { 'throwing: first error', // this repeated first error is the invokeGuardedCallback throw 'throwing: first error', - // these are actually thrown during render but no iGC repeat and no queueing as hydration errors - 'throwing: second error', - 'throwing: third error', - // all hydration errors are still queued + + // onRecoverableError because the UI recovered without surfacing the + // error to the user. 'Logged recoverable error: first error', - 'Logged recoverable error: second error', - 'Logged recoverable error: third error', - // other recoverable errors are queued as hydration errors 'Logged recoverable error: There was an error while hydrating this Suspense boundary. Switched to client rendering.', ]); // These Uncaught error calls are the error reported by the runtime (jsdom here, browser in actual use) @@ -4081,6 +4072,7 @@ describe('ReactDOMFizzServer', () => { } }); + // @gate __DEV__ it('does not invokeGuardedCallback for errors after a preceding fiber suspends', async () => { // We can't use the toErrorDev helper here because this is async. const originalConsoleError = console.error; @@ -4095,7 +4087,6 @@ describe('ReactDOMFizzServer', () => { mockError(...args.map(normalizeCodeLocInfo)); }; let isClient = false; - let shouldThrow = true; let promise = null; let unsuspend = null; let isResolved = false; @@ -4116,36 +4107,37 @@ describe('ReactDOMFizzServer', () => { return null; } - function ThrowUntilOnClient({children, message}) { - if (isClient && shouldThrow) { - Scheduler.log('throwing: ' + message); - throw new Error(message); - } + function ThrowWhenHydrating({children, message}) { + // This is a trick to only throw if we're hydrating, because + // useSyncExternalStore calls getServerSnapshot instead of the regular + // getSnapshot in that case. + useSyncExternalStore( + () => {}, + t => t, + () => { + if (isClient) { + Scheduler.log('throwing: ' + message); + throw new Error(message); + } + }, + ); return children; } - function StopThrowingOnClient() { - if (isClient) { - shouldThrow = false; - } - return null; - } - const App = () => { return (
Loading...}> - +

one

-
- + +

two

-
- + +

three

-
- +
); @@ -4172,15 +4164,7 @@ describe('ReactDOMFizzServer', () => { Scheduler.log('Logged recoverable error: ' + error.message); }, }); - await waitForAll([ - 'suspending', - 'throwing: first error', - // There is no repeated first error because we already suspended and no - // invokeGuardedCallback is used if we are in dev - // or in prod there is just never an invokeGuardedCallback - 'throwing: second error', - 'throwing: third error', - ]); + await waitForAll(['suspending']); expect(mockError.mock.calls).toEqual([]); expect(getVisibleChildren(container)).toEqual( @@ -4191,18 +4175,31 @@ describe('ReactDOMFizzServer', () => { , ); await unsuspend(); - // Since our client components only throw on the very first render there are no - // new throws in this pass - await waitForAll([]); - - expect(mockError.mock.calls).toEqual([]); + await waitForAll([ + 'throwing: first error', + 'throwing: first error', + 'Logged recoverable error: first error', + 'Logged recoverable error: There was an error while hydrating this Suspense boundary. Switched to client rendering.', + ]); + expect(getVisibleChildren(container)).toEqual( +
+

one

+

two

+

three

+
, + ); } finally { console.error = originalConsoleError; } }); // @gate __DEV__ - it('suspending after erroring will cause errors previously queued to be silenced until the boundary resolves', async () => { + it('(outdated behavior) suspending after erroring will cause errors previously queued to be silenced until the boundary resolves', async () => { + // NOTE: This test was originally written to test a scenario that doesn't happen + // anymore. If something errors during hydration, we immediately unwind the + // stack and revert to client rendering. I've kept the test around just to + // demonstrate what actually happens in this sequence of events. + // We can't use the toErrorDev helper here because this is async. const originalConsoleError = console.error; const mockError = jest.fn(); @@ -4216,7 +4213,6 @@ describe('ReactDOMFizzServer', () => { mockError(...args.map(normalizeCodeLocInfo)); }; let isClient = false; - let shouldThrow = true; let promise = null; let unsuspend = null; let isResolved = false; @@ -4237,36 +4233,37 @@ describe('ReactDOMFizzServer', () => { return null; } - function ThrowUntilOnClient({children, message}) { - if (isClient && shouldThrow) { - Scheduler.log('throwing: ' + message); - throw new Error(message); - } + function ThrowWhenHydrating({children, message}) { + // This is a trick to only throw if we're hydrating, because + // useSyncExternalStore calls getServerSnapshot instead of the regular + // getSnapshot in that case. + useSyncExternalStore( + () => {}, + t => t, + () => { + if (isClient) { + Scheduler.log('throwing: ' + message); + throw new Error(message); + } + }, + ); return children; } - function StopThrowingOnClient() { - if (isClient) { - shouldThrow = false; - } - return null; - } - const App = () => { return (
Loading...}> - +

one

-
- + +

two

-
+ - +

three

-
- +
); @@ -4297,9 +4294,9 @@ describe('ReactDOMFizzServer', () => { 'throwing: first error', // duplicate because first error is re-done in invokeGuardedCallback 'throwing: first error', - 'throwing: second error', 'suspending', - 'throwing: third error', + 'Logged recoverable error: first error', + 'Logged recoverable error: There was an error while hydrating this Suspense boundary. Switched to client rendering.', ]); // These Uncaught error calls are the error reported by the runtime (jsdom here, browser in actual use) // when invokeGuardedCallback is used to replay an error in dev using event dispatching in the document @@ -4312,9 +4309,7 @@ describe('ReactDOMFizzServer', () => { expect(getVisibleChildren(container)).toEqual(
-

one

-

two

-

three

+

Loading...

, ); await unsuspend(); @@ -4322,6 +4317,14 @@ describe('ReactDOMFizzServer', () => { // new throws in this pass await waitForAll([]); expect(mockError.mock.calls).toEqual([]); + + expect(getVisibleChildren(container)).toEqual( +
+

one

+

two

+

three

+
, + ); } finally { console.error = originalConsoleError; } diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js index b77c88ca5081b..b37225ba06bec 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js @@ -473,7 +473,6 @@ describe('ReactDOMFizzServerHydrationWarning', () => { }); await expect(async () => { await waitForAll([ - 'Hydration failed because the initial UI does not match what was rendered on the server.', 'Hydration failed because the initial UI does not match what was rendered on the server.', 'There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.', ]); diff --git a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js index bd34fbb20ce5b..3bdcd9c16a966 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js @@ -488,17 +488,16 @@ describe('ReactDOMServerHydration', () => { ); } expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` - [ - "Warning: Expected server HTML to contain a matching
in
. - in main (at **) - in div (at **) - in Mismatch (at **)", - "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", - "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", - "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", - "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", - ] - `); + [ + "Warning: Expected server HTML to contain a matching
in
. + in main (at **) + in div (at **) + in Mismatch (at **)", + "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", + "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", + "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", + ] + `); }); // @gate __DEV__ @@ -579,17 +578,16 @@ describe('ReactDOMServerHydration', () => { ); } expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` - [ - "Warning: Expected server HTML to contain a matching
in
. - in main (at **) - in div (at **) - in Mismatch (at **)", - "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", - "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", - "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", - "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", - ] - `); + [ + "Warning: Expected server HTML to contain a matching
in
. + in main (at **) + in div (at **) + in Mismatch (at **)", + "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", + "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", + "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", + ] + `); }); // @gate __DEV__ @@ -872,18 +870,16 @@ describe('ReactDOMServerHydration', () => { ); } expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` - [ - "Warning: Expected server HTML to contain a matching
in
. - in header (at **) - in div (at **) - in Mismatch (at **)", - "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", - "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", - "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", - "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", - "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", - ] - `); + [ + "Warning: Expected server HTML to contain a matching
in
. + in header (at **) + in div (at **) + in Mismatch (at **)", + "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", + "Caught [Hydration failed because the initial UI does not match what was rendered on the server.]", + "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", + ] + `); }); // @gate __DEV__ diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 7d98dbd87dc78..fb3341af1ce7b 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -310,13 +310,7 @@ describe('ReactDOMServerPartialHydration', () => { Scheduler.log(error.message); }, }); - await waitForAll([ - 'Suspend', - 'Component', - 'Component', - 'Component', - 'Component', - ]); + await waitForAll(['Suspend']); jest.runAllTimers(); // Unchanged @@ -1391,7 +1385,7 @@ describe('ReactDOMServerPartialHydration', () => { ); // This will throw it away and rerender. - await waitForAll(['Child', 'Sibling']); + await waitForAll(['Child']); expect(container.textContent).toBe('Hello'); diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 1431205b28f36..4124534f9c38a 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -1000,10 +1000,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Render third child, even though an earlier sibling threw. - 'Normal constructor', - 'Normal componentWillMount', - 'Normal render', + // Skip the remaining siblings // Handle the error 'ErrorBoundary static getDerivedStateFromError', 'ErrorBoundary componentWillMount', diff --git a/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js index fb5c827b3ae27..bcdf2b796134e 100644 --- a/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js @@ -1040,10 +1040,7 @@ describe('ReactLegacyErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Render third child, even though an earlier sibling threw. - 'Normal constructor', - 'Normal componentWillMount', - 'Normal render', + // Skip the remaining siblings // Finish mounting with null children 'ErrorBoundary componentDidMount', // Handle the error diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 4215cade8f87b..d11a50a856189 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -87,8 +87,6 @@ import { StaticMask, MutationMask, Passive, - Incomplete, - ShouldCapture, ForceClientRender, } from './ReactFiberFlags'; @@ -739,7 +737,7 @@ function completeDehydratedSuspenseBoundary( ) { warnIfUnhydratedTailNodes(workInProgress); resetHydrationState(); - workInProgress.flags |= ForceClientRender | Incomplete | ShouldCapture; + workInProgress.flags |= ForceClientRender | DidCapture; return false; } @@ -1146,7 +1144,7 @@ function completeWork( nextState, ); if (!fallthroughToNormalSuspensePath) { - if (workInProgress.flags & ShouldCapture) { + if (workInProgress.flags & ForceClientRender) { // Special case. There were remaining unhydrated nodes. We treat // this as a mismatch. Revert to client rendering. return workInProgress; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 53af9b4cae72d..54bc76a3d89ec 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -2528,16 +2528,15 @@ function completeUnitOfWork(unitOfWork: Fiber): void { // sibling. If there are no more siblings, return to the parent fiber. let completedWork: Fiber = unitOfWork; do { - if ((completedWork.flags & Incomplete) !== NoFlags) { - // This fiber did not complete, because one of its children did not - // complete. Switch to unwinding the stack instead of completing it. - // - // The reason "unwind" and "complete" is interleaved is because when - // something suspends, we continue rendering the siblings even though - // they will be replaced by a fallback. - // TODO: Disable sibling prerendering, then remove this branch. - unwindUnitOfWork(completedWork); - return; + if (__DEV__) { + if ((completedWork.flags & Incomplete) !== NoFlags) { + // NOTE: If we re-enable sibling prerendering in some cases, this branch + // is where we would switch to the unwinding path. + console.error( + 'Internal React error: Expected this fiber to be complete, but ' + + "it isn't. It should have been unwound. This is a bug in React.", + ); + } } // The current, flushed, state of this fiber is the alternate. Ideally @@ -2640,18 +2639,10 @@ function unwindUnitOfWork(unitOfWork: Fiber): void { returnFiber.deletions = null; } - // If there are siblings, work on them now even though they're going to be - // replaced by a fallback. We're "prerendering" them. Historically our - // rationale for this behavior has been to initiate any lazy data requests - // in the siblings, and also to warm up the CPU cache. - // TODO: Don't prerender siblings. With `use`, we suspend the work loop - // until the data has resolved, anyway. - const siblingFiber = incompleteWork.sibling; - if (siblingFiber !== null) { - // This branch will return us to the normal work loop. - workInProgress = siblingFiber; - return; - } + // NOTE: If we re-enable sibling prerendering in some cases, this branch + // is where we would switch to the normal completion path: check if a + // sibling exists, and if so, begin work on it. + // Otherwise, return to the parent // $FlowFixMe[incompatible-type] we bail out when we get a null incompleteWork = returnFiber; diff --git a/packages/react-reconciler/src/__tests__/ReactBatching-test.internal.js b/packages/react-reconciler/src/__tests__/ReactBatching-test.internal.js index d66634830fce7..15f5992109181 100644 --- a/packages/react-reconciler/src/__tests__/ReactBatching-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactBatching-test.internal.js @@ -111,7 +111,7 @@ describe('ReactBlockingMode', () => { , ); - await waitForAll(['A', 'Suspend! [B]', 'C', 'Loading...']); + await waitForAll(['A', 'Suspend! [B]', 'Loading...']); // In Legacy Mode, A and B would mount in a hidden primary tree. In // Concurrent Mode, nothing in the primary tree should mount. But the // fallback should mount immediately. diff --git a/packages/react-reconciler/src/__tests__/ReactCache-test.js b/packages/react-reconciler/src/__tests__/ReactCache-test.js index 6b1cb7423a4a9..7709652467e86 100644 --- a/packages/react-reconciler/src/__tests__/ReactCache-test.js +++ b/packages/react-reconciler/src/__tests__/ReactCache-test.js @@ -1057,7 +1057,7 @@ describe('ReactCache', () => { await act(() => { root.render(); }); - assertLog(['Cache miss! [A]', 'Cache miss! [B]', 'Loading...']); + assertLog(['Cache miss! [A]', 'Loading...']); expect(root).toMatchRenderedOutput('Loading...'); await act(() => { diff --git a/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js b/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js index e14fc83b2a3be..70adde03ddc5c 100644 --- a/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js +++ b/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js @@ -6,6 +6,7 @@ let Suspense; let getCacheForType; let startTransition; let assertLog; +let waitForPaint; let caches; let seededCache; @@ -23,6 +24,7 @@ describe('ReactConcurrentErrorRecovery', () => { const InternalTestUtils = require('internal-test-utils'); assertLog = InternalTestUtils.assertLog; + waitForPaint = InternalTestUtils.waitForPaint; getCacheForType = React.unstable_getCacheForType; @@ -450,20 +452,16 @@ describe('ReactConcurrentErrorRecovery', () => { await act(() => { startTransition(() => { root.render( - + <> - - , + + + + , ); }); }); - assertLog([ - 'Suspend! [Async]', - // TODO: Ideally we would skip this second render pass to render the - // error UI, since it's not going to commit anyway. The same goes for - // Suspense fallbacks during a refresh transition. - 'Caught an error: Oops!', - ]); + assertLog(['Suspend! [Async]']); // The render suspended without committing or surfacing the error. expect(root).toMatchRenderedOutput(null); @@ -471,14 +469,16 @@ describe('ReactConcurrentErrorRecovery', () => { await act(() => { startTransition(() => { root.render( - - + <> - , + + + + , ); }); }); - assertLog(['Suspend! [Async]', 'Caught an error: Oops!']); + assertLog(['Suspend! [Async]']); expect(root).toMatchRenderedOutput(null); await act(async () => { @@ -494,7 +494,7 @@ describe('ReactConcurrentErrorRecovery', () => { 'Caught an error: Oops!', ]); - expect(root).toMatchRenderedOutput('Caught an error: Oops!'); + expect(root).toMatchRenderedOutput('AsyncCaught an error: Oops!'); }, ); }); diff --git a/packages/react-reconciler/src/__tests__/ReactDisableSchedulerTimeoutBasedOnReactExpirationTime-test.internal.js b/packages/react-reconciler/src/__tests__/ReactDisableSchedulerTimeoutBasedOnReactExpirationTime-test.internal.js index ccde59f41d60f..c565f4a7b6d84 100644 --- a/packages/react-reconciler/src/__tests__/ReactDisableSchedulerTimeoutBasedOnReactExpirationTime-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactDisableSchedulerTimeoutBasedOnReactExpirationTime-test.internal.js @@ -72,7 +72,7 @@ describe('ReactSuspenseList', () => { React.startTransition(() => { root.render(); }); - await waitForAll(['Suspend! [A]', 'Suspend! [B]', 'Loading...']); + await waitForAll(['Suspend! [A]', 'Loading...']); expect(root).toMatchRenderedOutput(null); Scheduler.unstable_advanceTime(2000); diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index ae734e19711af..159e17d934623 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -647,7 +647,7 @@ describe('ReactExpiration', () => { React.startTransition(() => { root.render(); }); - await waitForAll(['Suspend! [A1]', 'B', 'C', 'Loading...']); + await waitForAll(['Suspend! [A1]', 'Loading...']); // Lots of time elapses before the promise resolves Scheduler.unstable_advanceTime(10000); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 2b274484bc625..2e7bdf0064080 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -99,17 +99,19 @@ describe('ReactIncrementalErrorHandling', () => { React.startTransition(() => { ReactNoop.render( - - + <> + - - - + + + - - , + + + + , ); }); @@ -121,13 +123,6 @@ describe('ReactIncrementalErrorHandling', () => { 'Indirection', // An error is thrown. React keeps rendering asynchronously. 'throw', - ]); - - // Still rendering async... - await waitFor(['Indirection']); - - await waitFor([ - 'Indirection', // Call getDerivedStateFromError and re-render the error boundary, this // time rendering an error message. @@ -135,14 +130,20 @@ describe('ReactIncrementalErrorHandling', () => { 'ErrorBoundary (catch)', 'ErrorMessage', ]); - - // Since the error was thrown during an async render, React won't commit - // the result yet. expect(ReactNoop).toMatchRenderedOutput(null); - // Instead, it will try rendering one more time, synchronously, in case that - // happens to fix the error. + // The work loop unwound to the nearest error boundary. Continue rendering + // asynchronously. + await waitFor(['Indirection']); + + // Since the error was thrown during an async render, React won't commit the + // result yet. After render we render the last child, React will attempt to + // render again, synchronously, just in case that happens to fix the error + // (i.e. as in the case of a data race). Flush just one more unit of work to + // demonstrate that this render is synchronous. expect(ReactNoop.flushNextYield()).toEqual([ + 'Indirection', + 'ErrorBoundary (try)', 'Indirection', 'Indirection', @@ -151,11 +152,11 @@ describe('ReactIncrementalErrorHandling', () => { // The error was thrown again. This time, React will actually commit // the result. 'throw', - 'Indirection', - 'Indirection', 'getDerivedStateFromError', 'ErrorBoundary (catch)', 'ErrorMessage', + 'Indirection', + 'Indirection', ]); expect(ReactNoop).toMatchRenderedOutput( @@ -197,17 +198,19 @@ describe('ReactIncrementalErrorHandling', () => { React.startTransition(() => { ReactNoop.render( - - + <> + - - - + + + - - , + + + + , ); }); @@ -384,12 +387,11 @@ describe('ReactIncrementalErrorHandling', () => { // Render the bad component asynchronously await waitFor(['Parent', 'BadRender']); - // Finish the rest of the async work - await waitFor(['Sibling']); - - // Old scheduler renders, commits, and throws synchronously + // The work loop unwound to the nearest error boundary. React will try + // to render one more time, synchronously. Flush just one unit of work to + // demonstrate that this render is synchronous. expect(() => Scheduler.unstable_flushNumberOfYields(1)).toThrow('oops'); - assertLog(['Parent', 'BadRender', 'Sibling', 'commit']); + assertLog(['Parent', 'BadRender', 'commit']); expect(ReactNoop).toMatchRenderedOutput(null); }); @@ -435,16 +437,12 @@ describe('ReactIncrementalErrorHandling', () => { // The render expired, but we shouldn't throw out the partial work. // Finish the current level. 'Oops', - 'C', - 'D', // Since the error occurred during a partially concurrent render, we should // retry one more time, synchronously. 'A', 'B', 'Oops', - 'C', - 'D', ]); expect(ReactNoop).toMatchRenderedOutput(null); }); @@ -1571,14 +1569,10 @@ describe('ReactIncrementalErrorHandling', () => { 'ErrorBoundary (try)', 'throw', // Continue rendering siblings after BadRender throws - 'BadRenderSibling', - 'BadRenderSibling', // React retries one more time 'ErrorBoundary (try)', 'throw', - 'BadRenderSibling', - 'BadRenderSibling', // Errored again on retry. Now handle it. 'componentDidCatch', diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index e033f63fa308b..f508ac4abf5b9 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -5,6 +5,7 @@ let Scheduler; let ReactFeatureFlags; let Suspense; let lazy; +let waitFor; let waitForAll; let waitForThrow; let assertLog; @@ -34,6 +35,7 @@ describe('ReactLazy', () => { Scheduler = require('scheduler'); const InternalTestUtils = require('internal-test-utils'); + waitFor = InternalTestUtils.waitFor; waitForAll = InternalTestUtils.waitForAll; waitForThrow = InternalTestUtils.waitForThrow; assertLog = InternalTestUtils.assertLog; @@ -256,8 +258,14 @@ describe('ReactLazy', () => { } } - const LazyChildA = lazy(() => fakeImport(Child)); - const LazyChildB = lazy(() => fakeImport(Child)); + const LazyChildA = lazy(() => { + Scheduler.log('Suspend! [LazyChildA]'); + return fakeImport(Child); + }); + const LazyChildB = lazy(() => { + Scheduler.log('Suspend! [LazyChildB]'); + return fakeImport(Child); + }); function Parent({swap}) { return ( @@ -279,11 +287,16 @@ describe('ReactLazy', () => { unstable_isConcurrent: true, }); - await waitForAll(['Loading...']); + await waitForAll(['Suspend! [LazyChildA]', 'Loading...']); expect(root).not.toMatchRenderedOutput('AB'); await resolveFakeImport(Child); + // B suspends even though it happens to share the same import as A. + // TODO: React.lazy should implement the `status` and `value` fields, so + // we can unwrap the result synchronously if it already loaded. Like `use`. + await waitFor(['A', 'Suspend! [LazyChildB]']); + await waitForAll(['A', 'B', 'Did mount: A', 'Did mount: B']); expect(root).toMatchRenderedOutput('AB'); @@ -1389,12 +1402,13 @@ describe('ReactLazy', () => { unstable_isConcurrent: true, }); - await waitForAll(['Init A', 'Init B', 'Loading...']); + await waitForAll(['Init A', 'Loading...']); expect(root).not.toMatchRenderedOutput('AB'); await resolveFakeImport(ChildA); - await resolveFakeImport(ChildB); + await waitForAll(['A', 'Init B']); + await resolveFakeImport(ChildB); await waitForAll(['A', 'B', 'Did mount: A', 'Did mount: B']); expect(root).toMatchRenderedOutput('AB'); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index d9a8f78d6bd27..df51d2b74d59e 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -134,8 +134,6 @@ describe('ReactSuspense', () => { 'Bar', // A suspends 'Suspend! [A]', - // But we keep rendering the siblings - 'B', 'Loading...', ]); expect(root).toMatchRenderedOutput(null); @@ -272,13 +270,7 @@ describe('ReactSuspense', () => { unstable_isConcurrent: true, }); - await waitForAll([ - 'Foo', - 'Suspend! [A]', - 'Suspend! [B]', - 'Loading more...', - 'Loading...', - ]); + await waitForAll(['Foo', 'Suspend! [A]', 'Loading...']); expect(root).toMatchRenderedOutput('Loading...'); await resolveText('A'); @@ -316,13 +308,7 @@ describe('ReactSuspense', () => { unstable_isConcurrent: true, }); - await waitForAll([ - 'Foo', - 'Suspend! [A]', - 'Suspend! [B]', - 'Loading more...', - 'Loading...', - ]); + await waitForAll(['Foo', 'Suspend! [A]', 'Loading...']); expect(root).toMatchRenderedOutput('Loading...'); await resolveText('A'); @@ -937,11 +923,7 @@ describe('ReactSuspense', () => { unstable_isConcurrent: true, }); - await waitForAll([ - 'Suspend! [Child 1]', - 'Suspend! [Child 2]', - 'Loading...', - ]); + await waitForAll(['Suspend! [Child 1]', 'Loading...']); await resolveText('Child 1'); await waitForAll(['Child 1', 'Suspend! [Child 2]']); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.js index 1f612eae67b90..5885ddb0a5969 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.js @@ -126,7 +126,7 @@ describe('ReactSuspense', () => { ReactNoop.render(element); await waitForAll([]); expect(ReactNoop).toMatchRenderedOutput('Waiting Tier 1'); - expect(ops).toEqual([new Set([promise1, promise2])]); + expect(ops).toEqual([new Set([promise1])]); ops = []; await resolve1(); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js index e8d81970ad5f7..defd59acf6423 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js @@ -266,7 +266,6 @@ describe('ReactSuspenseEffectsSemantics', () => { 'App render', 'Text:Inside:Before render', 'Suspend:Async', - 'ClassText:Inside:After render', 'Text:Fallback render', 'Text:Outside render', 'Text:Fallback create layout', @@ -641,7 +640,6 @@ describe('ReactSuspenseEffectsSemantics', () => { 'App render', 'Text:Inside:Before render', 'Suspend:Async', - 'Text:Inside:After render', 'Text:Fallback render', 'Text:Outside render', ]); @@ -796,7 +794,6 @@ describe('ReactSuspenseEffectsSemantics', () => { 'App render', 'ClassText:Inside:Before render', 'Suspend:Async', - 'ClassText:Inside:After render', 'ClassText:Fallback render', 'ClassText:Outside render', ]); @@ -917,13 +914,7 @@ describe('ReactSuspenseEffectsSemantics', () => { , ); - await waitFor([ - 'App render', - 'Suspend:Async', - 'Text:Outer render', - 'Text:Inner render', - 'Text:Fallback render', - ]); + await waitFor(['App render', 'Suspend:Async', 'Text:Fallback render']); expect(ReactNoop).toMatchRenderedOutput( @@ -1047,7 +1038,6 @@ describe('ReactSuspenseEffectsSemantics', () => { await waitFor([ 'App render', 'Suspend:Async', - 'Text:Outer render', // Text:MemoizedInner is memoized 'Text:Fallback render', ]); @@ -1186,9 +1176,6 @@ describe('ReactSuspenseEffectsSemantics', () => { assertLog([ 'Text:Outer render', 'Suspend:OuterAsync_1', - 'Text:Inner render', - 'Suspend:InnerAsync_1', - 'Text:InnerFallback render', 'Text:OuterFallback render', 'Text:Outer destroy layout', 'Text:InnerFallback destroy layout', @@ -1208,12 +1195,7 @@ describe('ReactSuspenseEffectsSemantics', () => { await act(async () => { await resolveText('InnerAsync_1'); }); - assertLog([ - 'Text:Outer render', - 'Suspend:OuterAsync_1', - 'Text:Inner render', - 'AsyncText:InnerAsync_1 render', - ]); + assertLog(['Text:Outer render', 'Suspend:OuterAsync_1']); expect(ReactNoop).toMatchRenderedOutput( <>