From 2a01cd0f008e531b273b8d6915e5385f164bf886 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 2 Nov 2022 10:53:23 -0700 Subject: [PATCH] Unwrap sync resolved thenables without suspending (#25615) If a thenable resolves synchronously, `use` should unwrap its result without suspending or interrupting the component's execution. --- .../src/__tests__/ReactDOMFizzServer-test.js | 27 ++++++++++++++ .../src/ReactFiberThenable.new.js | 18 ++++++++-- .../src/ReactFiberThenable.old.js | 18 ++++++++-- .../src/ReactFiberWorkLoop.new.js | 36 ++++++------------- .../src/ReactFiberWorkLoop.old.js | 36 ++++++------------- .../src/__tests__/ReactThenable-test.js | 28 +++++++++++++++ .../__tests__/ReactFlightDOMBrowser-test.js | 36 +++++++++++++++++++ .../react-server/src/ReactFizzThenable.js | 18 ++++++++-- .../react-server/src/ReactFlightThenable.js | 18 ++++++++-- 9 files changed, 171 insertions(+), 64 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 2333f366f50e5..7a4ca9d2464dd 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -5300,6 +5300,33 @@ describe('ReactDOMFizzServer', () => { expect(Scheduler).toFlushAndYield([]); expect(getVisibleChildren(container)).toEqual('Hi'); }); + + // @gate enableUseHook + it('unwraps thenable that fulfills synchronously without suspending', async () => { + function App() { + const thenable = { + then(resolve) { + // This thenable immediately resolves, synchronously, without waiting + // a microtask. + resolve('Hi'); + }, + }; + try { + return ; + } catch { + throw new Error( + '`use` should not suspend because the thenable resolved synchronously.', + ); + } + } + // Because the thenable resolves synchronously, we should be able to finish + // rendering synchronously, with no fallback. + await act(async () => { + const {pipe} = renderToPipeableStream(); + pipe(writable); + }); + expect(getVisibleChildren(container)).toEqual('Hi'); + }); }); describe('useEvent', () => { diff --git a/packages/react-reconciler/src/ReactFiberThenable.new.js b/packages/react-reconciler/src/ReactFiberThenable.new.js index d10c4d37e9ac2..c884737ae5de7 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.new.js +++ b/packages/react-reconciler/src/ReactFiberThenable.new.js @@ -110,24 +110,36 @@ export function trackUsedThenable(thenable: Thenable, index: number): T { // it's defined, but an unknown value, assume it's been instrumented by // some custom userspace implementation. We treat it as "pending". } else { - const pendingThenable: PendingThenable = (thenable: any); + const pendingThenable: PendingThenable = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then( fulfilledValue => { if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); + const fulfilledThenable: FulfilledThenable = (thenable: any); fulfilledThenable.status = 'fulfilled'; fulfilledThenable.value = fulfilledValue; } }, (error: mixed) => { if (thenable.status === 'pending') { - const rejectedThenable: RejectedThenable = (thenable: any); + const rejectedThenable: RejectedThenable = (thenable: any); rejectedThenable.status = 'rejected'; rejectedThenable.reason = error; } }, ); + + // Check one more time in case the thenable resolved synchronously + switch (thenable.status) { + case 'fulfilled': { + const fulfilledThenable: FulfilledThenable = (thenable: any); + return fulfilledThenable.value; + } + case 'rejected': { + const rejectedThenable: RejectedThenable = (thenable: any); + throw rejectedThenable.reason; + } + } } // Suspend. diff --git a/packages/react-reconciler/src/ReactFiberThenable.old.js b/packages/react-reconciler/src/ReactFiberThenable.old.js index d10c4d37e9ac2..c884737ae5de7 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.old.js +++ b/packages/react-reconciler/src/ReactFiberThenable.old.js @@ -110,24 +110,36 @@ export function trackUsedThenable(thenable: Thenable, index: number): T { // it's defined, but an unknown value, assume it's been instrumented by // some custom userspace implementation. We treat it as "pending". } else { - const pendingThenable: PendingThenable = (thenable: any); + const pendingThenable: PendingThenable = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then( fulfilledValue => { if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); + const fulfilledThenable: FulfilledThenable = (thenable: any); fulfilledThenable.status = 'fulfilled'; fulfilledThenable.value = fulfilledValue; } }, (error: mixed) => { if (thenable.status === 'pending') { - const rejectedThenable: RejectedThenable = (thenable: any); + const rejectedThenable: RejectedThenable = (thenable: any); rejectedThenable.status = 'rejected'; rejectedThenable.reason = error; } }, ); + + // Check one more time in case the thenable resolved synchronously + switch (thenable.status) { + case 'fulfilled': { + const fulfilledThenable: FulfilledThenable = (thenable: any); + return fulfilledThenable.value; + } + case 'rejected': { + const rejectedThenable: RejectedThenable = (thenable: any); + throw rejectedThenable.reason; + } + } } // Suspend. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index c7326aaafa28c..1eac6957f4e8b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2016,37 +2016,21 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { workInProgressSuspendedReason !== NotSuspended && workInProgress !== null ) { - // The work loop is suspended. We need to either unwind the stack or - // replay the suspended component. + // The work loop is suspended. During a synchronous render, we don't + // yield to the main thread. Immediately unwind the stack. This will + // trigger either a fallback or an error boundary. + // TODO: For discrete and "default" updates (anything that's not + // flushSync), we want to wait for the microtasks the flush before + // unwinding. Will probably implement this using renderRootConcurrent, + // or merge renderRootSync and renderRootConcurrent into the same + // function and fork the behavior some other way. const unitOfWork = workInProgress; const thrownValue = workInProgressThrownValue; workInProgressSuspendedReason = NotSuspended; workInProgressThrownValue = null; + unwindSuspendedUnitOfWork(unitOfWork, thrownValue); - // TODO: This check is only here to account for thenables that - // synchronously resolve. Otherwise we would always unwind when - // rendering with renderRootSync. (In the future, discrete updates will - // use renderRootConcurrent instead.) We should account for - // synchronously resolved thenables before hitting this path. - switch (workInProgressSuspendedReason) { - case SuspendedOnError: { - // Unwind then continue with the normal work loop. - unwindSuspendedUnitOfWork(unitOfWork, thrownValue); - break; - } - default: { - const wasPinged = - workInProgressSuspendedThenableState !== null && - isThenableStateResolved(workInProgressSuspendedThenableState); - if (wasPinged) { - replaySuspendedUnitOfWork(unitOfWork, thrownValue); - } else { - unwindSuspendedUnitOfWork(unitOfWork, thrownValue); - } - // Continue with the normal work loop. - break; - } - } + // Continue with the normal work loop. } workLoopSync(); break; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 04f835ee9bf56..0b62d0f2a3099 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2016,37 +2016,21 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { workInProgressSuspendedReason !== NotSuspended && workInProgress !== null ) { - // The work loop is suspended. We need to either unwind the stack or - // replay the suspended component. + // The work loop is suspended. During a synchronous render, we don't + // yield to the main thread. Immediately unwind the stack. This will + // trigger either a fallback or an error boundary. + // TODO: For discrete and "default" updates (anything that's not + // flushSync), we want to wait for the microtasks the flush before + // unwinding. Will probably implement this using renderRootConcurrent, + // or merge renderRootSync and renderRootConcurrent into the same + // function and fork the behavior some other way. const unitOfWork = workInProgress; const thrownValue = workInProgressThrownValue; workInProgressSuspendedReason = NotSuspended; workInProgressThrownValue = null; + unwindSuspendedUnitOfWork(unitOfWork, thrownValue); - // TODO: This check is only here to account for thenables that - // synchronously resolve. Otherwise we would always unwind when - // rendering with renderRootSync. (In the future, discrete updates will - // use renderRootConcurrent instead.) We should account for - // synchronously resolved thenables before hitting this path. - switch (workInProgressSuspendedReason) { - case SuspendedOnError: { - // Unwind then continue with the normal work loop. - unwindSuspendedUnitOfWork(unitOfWork, thrownValue); - break; - } - default: { - const wasPinged = - workInProgressSuspendedThenableState !== null && - isThenableStateResolved(workInProgressSuspendedThenableState); - if (wasPinged) { - replaySuspendedUnitOfWork(unitOfWork, thrownValue); - } else { - unwindSuspendedUnitOfWork(unitOfWork, thrownValue); - } - // Continue with the normal work loop. - break; - } - } + // Continue with the normal work loop. } workLoopSync(); break; diff --git a/packages/react-reconciler/src/__tests__/ReactThenable-test.js b/packages/react-reconciler/src/__tests__/ReactThenable-test.js index 887cd26080d8f..f0bca1420145c 100644 --- a/packages/react-reconciler/src/__tests__/ReactThenable-test.js +++ b/packages/react-reconciler/src/__tests__/ReactThenable-test.js @@ -640,4 +640,32 @@ describe('ReactThenable', () => { }); expect(Scheduler).toHaveYielded(['Something different']); }); + + // @gate enableUseHook + test('unwraps thenable that fulfills synchronously without suspending', async () => { + function App() { + const thenable = { + then(resolve) { + // This thenable immediately resolves, synchronously, without waiting + // a microtask. + resolve('Hi'); + }, + }; + try { + return ; + } catch { + throw new Error( + '`use` should not suspend because the thenable resolved synchronously.', + ); + } + } + // Because the thenable resolves synchronously, we should be able to finish + // rendering synchronously, with no fallback. + const root = ReactNoop.createRoot(); + ReactNoop.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Hi']); + expect(root).toMatchRenderedOutput('Hi'); + }); }); diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index 54b98f5351a14..05b7dd63d6dec 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -778,4 +778,40 @@ describe('ReactFlightDOMBrowser', () => { }); expect(container.innerHTML).toBe('Hi'); }); + + // @gate enableUseHook + it('unwraps thenable that fulfills synchronously without suspending', async () => { + function Server() { + const thenable = { + then(resolve) { + // This thenable immediately resolves, synchronously, without waiting + // a microtask. + resolve('Hi'); + }, + }; + try { + return use(thenable); + } catch { + throw new Error( + '`use` should not suspend because the thenable resolved synchronously.', + ); + } + } + + // Because the thenable resolves synchronously, we should be able to finish + // rendering synchronously, with no fallback. + const stream = ReactServerDOMWriter.renderToReadableStream(); + const response = ReactServerDOMReader.createFromReadableStream(stream); + + function Client() { + return use(response); + } + + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + await act(async () => { + root.render(); + }); + expect(container.innerHTML).toBe('Hi'); + }); }); diff --git a/packages/react-server/src/ReactFizzThenable.js b/packages/react-server/src/ReactFizzThenable.js index 415ddc119c781..b863d8b6f9f99 100644 --- a/packages/react-server/src/ReactFizzThenable.js +++ b/packages/react-server/src/ReactFizzThenable.js @@ -83,24 +83,36 @@ export function trackUsedThenable( // it's defined, but an unknown value, assume it's been instrumented by // some custom userspace implementation. We treat it as "pending". } else { - const pendingThenable: PendingThenable = (thenable: any); + const pendingThenable: PendingThenable = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then( fulfilledValue => { if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); + const fulfilledThenable: FulfilledThenable = (thenable: any); fulfilledThenable.status = 'fulfilled'; fulfilledThenable.value = fulfilledValue; } }, (error: mixed) => { if (thenable.status === 'pending') { - const rejectedThenable: RejectedThenable = (thenable: any); + const rejectedThenable: RejectedThenable = (thenable: any); rejectedThenable.status = 'rejected'; rejectedThenable.reason = error; } }, ); + + // Check one more time in case the thenable resolved synchronously + switch (thenable.status) { + case 'fulfilled': { + const fulfilledThenable: FulfilledThenable = (thenable: any); + return fulfilledThenable.value; + } + case 'rejected': { + const rejectedThenable: RejectedThenable = (thenable: any); + throw rejectedThenable.reason; + } + } } // Suspend. diff --git a/packages/react-server/src/ReactFlightThenable.js b/packages/react-server/src/ReactFlightThenable.js index 02d24b1c7665c..852c13b2be4e4 100644 --- a/packages/react-server/src/ReactFlightThenable.js +++ b/packages/react-server/src/ReactFlightThenable.js @@ -83,24 +83,36 @@ export function trackUsedThenable( // it's defined, but an unknown value, assume it's been instrumented by // some custom userspace implementation. We treat it as "pending". } else { - const pendingThenable: PendingThenable = (thenable: any); + const pendingThenable: PendingThenable = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then( fulfilledValue => { if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); + const fulfilledThenable: FulfilledThenable = (thenable: any); fulfilledThenable.status = 'fulfilled'; fulfilledThenable.value = fulfilledValue; } }, (error: mixed) => { if (thenable.status === 'pending') { - const rejectedThenable: RejectedThenable = (thenable: any); + const rejectedThenable: RejectedThenable = (thenable: any); rejectedThenable.status = 'rejected'; rejectedThenable.reason = error; } }, ); + + // Check one more time in case the thenable resolved synchronously + switch (thenable.status) { + case 'fulfilled': { + const fulfilledThenable: FulfilledThenable = (thenable: any); + return fulfilledThenable.value; + } + case 'rejected': { + const rejectedThenable: RejectedThenable = (thenable: any); + throw rejectedThenable.reason; + } + } } // Suspend.