{"commit":{"authoredAt":"2022-11-02T10:53:23-07:00","shortRevision":"1a902623a8","revision":"1a902623a84ab004f9f042162bdcbce20f08b13f","authorName":"Andrew Clark","subject":"Unwrap sync resolved thenables without suspending (#25615)","authorEmail":"git@andrewclark.io"},"files":[{"status":"M","path":"packages\/react-dom\/src\/__tests__\/ReactDOMFizzServer-test.js"},{"status":"M","path":"packages\/react-reconciler\/src\/ReactFiberThenable.new.js"},{"status":"M","path":"packages\/react-reconciler\/src\/ReactFiberThenable.old.js"},{"status":"M","path":"packages\/react-reconciler\/src\/ReactFiberWorkLoop.new.js"},{"status":"M","path":"packages\/react-reconciler\/src\/ReactFiberWorkLoop.old.js"},{"status":"M","path":"packages\/react-reconciler\/src\/__tests__\/ReactThenable-test.js"},{"status":"M","path":"packages\/react-server-dom-webpack\/src\/__tests__\/ReactFlightDOMBrowser-test.js"},{"status":"M","path":"packages\/react-server\/src\/ReactFizzThenable.js"},{"status":"M","path":"packages\/react-server\/src\/ReactFlightThenable.js"}],"body":"If a thenable resolves synchronously, `use` should unwrap its result\r\nwithout suspending or interrupting the component's execution.","patch":"diff --git a\/packages\/react-dom\/src\/__tests__\/ReactDOMFizzServer-test.js b\/packages\/react-dom\/src\/__tests__\/ReactDOMFizzServer-test.js\nindex 2333f366f5..7a4ca9d246 100644\n--- a\/packages\/react-dom\/src\/__tests__\/ReactDOMFizzServer-test.js\n+++ b\/packages\/react-dom\/src\/__tests__\/ReactDOMFizzServer-test.js\n@@ -5300,6 +5300,33 @@ describe('ReactDOMFizzServer', () => {\n expect(Scheduler).toFlushAndYield([]);\n expect(getVisibleChildren(container)).toEqual('Hi');\n });\n+\n+ \/\/ @gate enableUseHook\n+ it('unwraps thenable that fulfills synchronously without suspending', async () => {\n+ function App() {\n+ const thenable = {\n+ then(resolve) {\n+ \/\/ This thenable immediately resolves, synchronously, without waiting\n+ \/\/ a microtask.\n+ resolve('Hi');\n+ },\n+ };\n+ try {\n+ return ;\n+ } catch {\n+ throw new Error(\n+ '`use` should not suspend because the thenable resolved synchronously.',\n+ );\n+ }\n+ }\n+ \/\/ Because the thenable resolves synchronously, we should be able to finish\n+ \/\/ rendering synchronously, with no fallback.\n+ await act(async () => {\n+ const {pipe} = renderToPipeableStream();\n+ pipe(writable);\n+ });\n+ expect(getVisibleChildren(container)).toEqual('Hi');\n+ });\n });\n \n describe('useEvent', () => {\ndiff --git a\/packages\/react-reconciler\/src\/ReactFiberThenable.new.js b\/packages\/react-reconciler\/src\/ReactFiberThenable.new.js\nindex d10c4d37e9..c884737ae5 100644\n--- a\/packages\/react-reconciler\/src\/ReactFiberThenable.new.js\n+++ b\/packages\/react-reconciler\/src\/ReactFiberThenable.new.js\n@@ -110,24 +110,36 @@ export function trackUsedThenable(thenable: Thenable, index: number): T {\n \/\/ it's defined, but an unknown value, assume it's been instrumented by\n \/\/ some custom userspace implementation. We treat it as \"pending\".\n } else {\n- const pendingThenable: PendingThenable = (thenable: any);\n+ const pendingThenable: PendingThenable = (thenable: any);\n pendingThenable.status = 'pending';\n pendingThenable.then(\n fulfilledValue => {\n if (thenable.status === 'pending') {\n- const fulfilledThenable: FulfilledThenable = (thenable: any);\n+ const fulfilledThenable: FulfilledThenable = (thenable: any);\n fulfilledThenable.status = 'fulfilled';\n fulfilledThenable.value = fulfilledValue;\n }\n },\n (error: mixed) => {\n if (thenable.status === 'pending') {\n- const rejectedThenable: RejectedThenable = (thenable: any);\n+ const rejectedThenable: RejectedThenable = (thenable: any);\n rejectedThenable.status = 'rejected';\n rejectedThenable.reason = error;\n }\n },\n );\n+\n+ \/\/ Check one more time in case the thenable resolved synchronously\n+ switch (thenable.status) {\n+ case 'fulfilled': {\n+ const fulfilledThenable: FulfilledThenable = (thenable: any);\n+ return fulfilledThenable.value;\n+ }\n+ case 'rejected': {\n+ const rejectedThenable: RejectedThenable = (thenable: any);\n+ throw rejectedThenable.reason;\n+ }\n+ }\n }\n \n \/\/ Suspend.\ndiff --git a\/packages\/react-reconciler\/src\/ReactFiberThenable.old.js b\/packages\/react-reconciler\/src\/ReactFiberThenable.old.js\nindex d10c4d37e9..c884737ae5 100644\n--- a\/packages\/react-reconciler\/src\/ReactFiberThenable.old.js\n+++ b\/packages\/react-reconciler\/src\/ReactFiberThenable.old.js\n@@ -110,24 +110,36 @@ export function trackUsedThenable(thenable: Thenable, index: number): T {\n \/\/ it's defined, but an unknown value, assume it's been instrumented by\n \/\/ some custom userspace implementation. We treat it as \"pending\".\n } else {\n- const pendingThenable: PendingThenable = (thenable: any);\n+ const pendingThenable: PendingThenable = (thenable: any);\n pendingThenable.status = 'pending';\n pendingThenable.then(\n fulfilledValue => {\n if (thenable.status === 'pending') {\n- const fulfilledThenable: FulfilledThenable = (thenable: any);\n+ const fulfilledThenable: FulfilledThenable = (thenable: any);\n fulfilledThenable.status = 'fulfilled';\n fulfilledThenable.value = fulfilledValue;\n }\n },\n (error: mixed) => {\n if (thenable.status === 'pending') {\n- const rejectedThenable: RejectedThenable = (thenable: any);\n+ const rejectedThenable: RejectedThenable = (thenable: any);\n rejectedThenable.status = 'rejected';\n rejectedThenable.reason = error;\n }\n },\n );\n+\n+ \/\/ Check one more time in case the thenable resolved synchronously\n+ switch (thenable.status) {\n+ case 'fulfilled': {\n+ const fulfilledThenable: FulfilledThenable = (thenable: any);\n+ return fulfilledThenable.value;\n+ }\n+ case 'rejected': {\n+ const rejectedThenable: RejectedThenable = (thenable: any);\n+ throw rejectedThenable.reason;\n+ }\n+ }\n }\n \n \/\/ Suspend.\ndiff --git a\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.new.js b\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.new.js\nindex c7326aaafa..1eac6957f4 100644\n--- a\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.new.js\n+++ b\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.new.js\n@@ -2016,37 +2016,21 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) {\n workInProgressSuspendedReason !== NotSuspended &&\n workInProgress !== null\n ) {\n- \/\/ The work loop is suspended. We need to either unwind the stack or\n- \/\/ replay the suspended component.\n+ \/\/ The work loop is suspended. During a synchronous render, we don't\n+ \/\/ yield to the main thread. Immediately unwind the stack. This will\n+ \/\/ trigger either a fallback or an error boundary.\n+ \/\/ TODO: For discrete and \"default\" updates (anything that's not\n+ \/\/ flushSync), we want to wait for the microtasks the flush before\n+ \/\/ unwinding. Will probably implement this using renderRootConcurrent,\n+ \/\/ or merge renderRootSync and renderRootConcurrent into the same\n+ \/\/ function and fork the behavior some other way.\n const unitOfWork = workInProgress;\n const thrownValue = workInProgressThrownValue;\n workInProgressSuspendedReason = NotSuspended;\n workInProgressThrownValue = null;\n+ unwindSuspendedUnitOfWork(unitOfWork, thrownValue);\n \n- \/\/ TODO: This check is only here to account for thenables that\n- \/\/ synchronously resolve. Otherwise we would always unwind when\n- \/\/ rendering with renderRootSync. (In the future, discrete updates will\n- \/\/ use renderRootConcurrent instead.) We should account for\n- \/\/ synchronously resolved thenables before hitting this path.\n- switch (workInProgressSuspendedReason) {\n- case SuspendedOnError: {\n- \/\/ Unwind then continue with the normal work loop.\n- unwindSuspendedUnitOfWork(unitOfWork, thrownValue);\n- break;\n- }\n- default: {\n- const wasPinged =\n- workInProgressSuspendedThenableState !== null &&\n- isThenableStateResolved(workInProgressSuspendedThenableState);\n- if (wasPinged) {\n- replaySuspendedUnitOfWork(unitOfWork, thrownValue);\n- } else {\n- unwindSuspendedUnitOfWork(unitOfWork, thrownValue);\n- }\n- \/\/ Continue with the normal work loop.\n- break;\n- }\n- }\n+ \/\/ Continue with the normal work loop.\n }\n workLoopSync();\n break;\ndiff --git a\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.old.js b\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.old.js\nindex 04f835ee9b..0b62d0f2a3 100644\n--- a\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.old.js\n+++ b\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.old.js\n@@ -2016,37 +2016,21 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) {\n workInProgressSuspendedReason !== NotSuspended &&\n workInProgress !== null\n ) {\n- \/\/ The work loop is suspended. We need to either unwind the stack or\n- \/\/ replay the suspended component.\n+ \/\/ The work loop is suspended. During a synchronous render, we don't\n+ \/\/ yield to the main thread. Immediately unwind the stack. This will\n+ \/\/ trigger either a fallback or an error boundary.\n+ \/\/ TODO: For discrete and \"default\" updates (anything that's not\n+ \/\/ flushSync), we want to wait for the microtasks the flush before\n+ \/\/ unwinding. Will probably implement this using renderRootConcurrent,\n+ \/\/ or merge renderRootSync and renderRootConcurrent into the same\n+ \/\/ function and fork the behavior some other way.\n const unitOfWork = workInProgress;\n const thrownValue = workInProgressThrownValue;\n workInProgressSuspendedReason = NotSuspended;\n workInProgressThrownValue = null;\n+ unwindSuspendedUnitOfWork(unitOfWork, thrownValue);\n \n- \/\/ TODO: This check is only here to account for thenables that\n- \/\/ synchronously resolve. Otherwise we would always unwind when\n- \/\/ rendering with renderRootSync. (In the future, discrete updates will\n- \/\/ use renderRootConcurrent instead.) We should account for\n- \/\/ synchronously resolved thenables before hitting this path.\n- switch (workInProgressSuspendedReason) {\n- case SuspendedOnError: {\n- \/\/ Unwind then continue with the normal work loop.\n- unwindSuspendedUnitOfWork(unitOfWork, thrownValue);\n- break;\n- }\n- default: {\n- const wasPinged =\n- workInProgressSuspendedThenableState !== null &&\n- isThenableStateResolved(workInProgressSuspendedThenableState);\n- if (wasPinged) {\n- replaySuspendedUnitOfWork(unitOfWork, thrownValue);\n- } else {\n- unwindSuspendedUnitOfWork(unitOfWork, thrownValue);\n- }\n- \/\/ Continue with the normal work loop.\n- break;\n- }\n- }\n+ \/\/ Continue with the normal work loop.\n }\n workLoopSync();\n break;\ndiff --git a\/packages\/react-reconciler\/src\/__tests__\/ReactThenable-test.js b\/packages\/react-reconciler\/src\/__tests__\/ReactThenable-test.js\nindex 887cd26080..f0bca14201 100644\n--- a\/packages\/react-reconciler\/src\/__tests__\/ReactThenable-test.js\n+++ b\/packages\/react-reconciler\/src\/__tests__\/ReactThenable-test.js\n@@ -640,4 +640,32 @@ describe('ReactThenable', () => {\n });\n expect(Scheduler).toHaveYielded(['Something different']);\n });\n+\n+ \/\/ @gate enableUseHook\n+ test('unwraps thenable that fulfills synchronously without suspending', async () => {\n+ function App() {\n+ const thenable = {\n+ then(resolve) {\n+ \/\/ This thenable immediately resolves, synchronously, without waiting\n+ \/\/ a microtask.\n+ resolve('Hi');\n+ },\n+ };\n+ try {\n+ return ;\n+ } catch {\n+ throw new Error(\n+ '`use` should not suspend because the thenable resolved synchronously.',\n+ );\n+ }\n+ }\n+ \/\/ Because the thenable resolves synchronously, we should be able to finish\n+ \/\/ rendering synchronously, with no fallback.\n+ const root = ReactNoop.createRoot();\n+ ReactNoop.flushSync(() => {\n+ root.render();\n+ });\n+ expect(Scheduler).toHaveYielded(['Hi']);\n+ expect(root).toMatchRenderedOutput('Hi');\n+ });\n });\ndiff --git a\/packages\/react-server-dom-webpack\/src\/__tests__\/ReactFlightDOMBrowser-test.js b\/packages\/react-server-dom-webpack\/src\/__tests__\/ReactFlightDOMBrowser-test.js\nindex 54b98f5351..05b7dd63d6 100644\n--- a\/packages\/react-server-dom-webpack\/src\/__tests__\/ReactFlightDOMBrowser-test.js\n+++ b\/packages\/react-server-dom-webpack\/src\/__tests__\/ReactFlightDOMBrowser-test.js\n@@ -778,4 +778,40 @@ describe('ReactFlightDOMBrowser', () => {\n });\n expect(container.innerHTML).toBe('Hi');\n });\n+\n+ \/\/ @gate enableUseHook\n+ it('unwraps thenable that fulfills synchronously without suspending', async () => {\n+ function Server() {\n+ const thenable = {\n+ then(resolve) {\n+ \/\/ This thenable immediately resolves, synchronously, without waiting\n+ \/\/ a microtask.\n+ resolve('Hi');\n+ },\n+ };\n+ try {\n+ return use(thenable);\n+ } catch {\n+ throw new Error(\n+ '`use` should not suspend because the thenable resolved synchronously.',\n+ );\n+ }\n+ }\n+\n+ \/\/ Because the thenable resolves synchronously, we should be able to finish\n+ \/\/ rendering synchronously, with no fallback.\n+ const stream = ReactServerDOMWriter.renderToReadableStream();\n+ const response = ReactServerDOMReader.createFromReadableStream(stream);\n+\n+ function Client() {\n+ return use(response);\n+ }\n+\n+ const container = document.createElement('div');\n+ const root = ReactDOMClient.createRoot(container);\n+ await act(async () => {\n+ root.render();\n+ });\n+ expect(container.innerHTML).toBe('Hi');\n+ });\n });\ndiff --git a\/packages\/react-server\/src\/ReactFizzThenable.js b\/packages\/react-server\/src\/ReactFizzThenable.js\nindex 415ddc119c..b863d8b6f9 100644\n--- a\/packages\/react-server\/src\/ReactFizzThenable.js\n+++ b\/packages\/react-server\/src\/ReactFizzThenable.js\n@@ -83,24 +83,36 @@ export function trackUsedThenable(\n \/\/ it's defined, but an unknown value, assume it's been instrumented by\n \/\/ some custom userspace implementation. We treat it as \"pending\".\n } else {\n- const pendingThenable: PendingThenable = (thenable: any);\n+ const pendingThenable: PendingThenable = (thenable: any);\n pendingThenable.status = 'pending';\n pendingThenable.then(\n fulfilledValue => {\n if (thenable.status === 'pending') {\n- const fulfilledThenable: FulfilledThenable = (thenable: any);\n+ const fulfilledThenable: FulfilledThenable = (thenable: any);\n fulfilledThenable.status = 'fulfilled';\n fulfilledThenable.value = fulfilledValue;\n }\n },\n (error: mixed) => {\n if (thenable.status === 'pending') {\n- const rejectedThenable: RejectedThenable = (thenable: any);\n+ const rejectedThenable: RejectedThenable = (thenable: any);\n rejectedThenable.status = 'rejected';\n rejectedThenable.reason = error;\n }\n },\n );\n+\n+ \/\/ Check one more time in case the thenable resolved synchronously\n+ switch (thenable.status) {\n+ case 'fulfilled': {\n+ const fulfilledThenable: FulfilledThenable = (thenable: any);\n+ return fulfilledThenable.value;\n+ }\n+ case 'rejected': {\n+ const rejectedThenable: RejectedThenable = (thenable: any);\n+ throw rejectedThenable.reason;\n+ }\n+ }\n }\n \n \/\/ Suspend.\ndiff --git a\/packages\/react-server\/src\/ReactFlightThenable.js b\/packages\/react-server\/src\/ReactFlightThenable.js\nindex 02d24b1c76..852c13b2be 100644\n--- a\/packages\/react-server\/src\/ReactFlightThenable.js\n+++ b\/packages\/react-server\/src\/ReactFlightThenable.js\n@@ -83,24 +83,36 @@ export function trackUsedThenable(\n \/\/ it's defined, but an unknown value, assume it's been instrumented by\n \/\/ some custom userspace implementation. We treat it as \"pending\".\n } else {\n- const pendingThenable: PendingThenable = (thenable: any);\n+ const pendingThenable: PendingThenable = (thenable: any);\n pendingThenable.status = 'pending';\n pendingThenable.then(\n fulfilledValue => {\n if (thenable.status === 'pending') {\n- const fulfilledThenable: FulfilledThenable = (thenable: any);\n+ const fulfilledThenable: FulfilledThenable = (thenable: any);\n fulfilledThenable.status = 'fulfilled';\n fulfilledThenable.value = fulfilledValue;\n }\n },\n (error: mixed) => {\n if (thenable.status === 'pending') {\n- const rejectedThenable: RejectedThenable = (thenable: any);\n+ const rejectedThenable: RejectedThenable = (thenable: any);\n rejectedThenable.status = 'rejected';\n rejectedThenable.reason = error;\n }\n },\n );\n+\n+ \/\/ Check one more time in case the thenable resolved synchronously\n+ switch (thenable.status) {\n+ case 'fulfilled': {\n+ const fulfilledThenable: FulfilledThenable = (thenable: any);\n+ return fulfilledThenable.value;\n+ }\n+ case 'rejected': {\n+ const rejectedThenable: RejectedThenable = (thenable: any);\n+ throw rejectedThenable.reason;\n+ }\n+ }\n }\n \n \/\/ Suspend.\n","parents":["4ea063b56ff7ccc69c4cc67c30fd6f31f9880464"]}

dillon1000/react

Files changed