From 91ac3b9764e8df085ccba48471240d8fec19ad26 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Mon, 8 Apr 2024 12:42:52 +0200 Subject: [PATCH 1/3] Add Promise as a child to Flight fixture --- fixtures/flight/__tests__/__e2e__/smoke.test.js | 4 +++- fixtures/flight/src/App.js | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/fixtures/flight/__tests__/__e2e__/smoke.test.js b/fixtures/flight/__tests__/__e2e__/smoke.test.js index af889d5de16a..267bd109081a 100644 --- a/fixtures/flight/__tests__/__e2e__/smoke.test.js +++ b/fixtures/flight/__tests__/__e2e__/smoke.test.js @@ -13,7 +13,9 @@ test('smoke test', async ({page}) => { pageErrors.push(error.stack); }); await page.goto('/'); - await expect(page.locator('h1')).toHaveText('Hello World'); + await expect(page.getByTestId('promise-as-a-child-test')).toHaveText( + 'Promise as a child hydrates without errors: deferred text' + ); await expect(consoleErrors).toEqual([]); await expect(pageErrors).toEqual([]); diff --git a/fixtures/flight/src/App.js b/fixtures/flight/src/App.js index 7a14beb460de..027056c51502 100644 --- a/fixtures/flight/src/App.js +++ b/fixtures/flight/src/App.js @@ -19,6 +19,10 @@ import {like, greet, increment} from './actions.js'; import {getServerState} from './ServerState.js'; +const promisedText = new Promise(resolve => + setTimeout(() => resolve('deferred text'), 100) +); + export default async function App() { const res = await fetch('http://localhost:3001/todos'); const todos = await res.json(); @@ -32,6 +36,11 @@ export default async function App() {

{getServerState()}

+ +
+ Promise as a child hydrates without errors: {promisedText} +
+ From 74b1848d1d33ca82fa75b43910cb3071b7923b7a Mon Sep 17 00:00:00 2001 From: eps1lon Date: Mon, 8 Apr 2024 12:44:03 +0200 Subject: [PATCH 2/3] Revert "Suspend Thenable/Lazy if it's used in React.Children and unwrap (#28284)" This reverts commit 9e7944f67c72b9a69a8db092ba6bd99fe9c731e2. --- .../src/ReactFiberThenable.js | 24 ++--- packages/react/src/ReactChildren.js | 96 ++----------------- .../react/src/__tests__/ReactChildren-test.js | 42 ++++---- 3 files changed, 38 insertions(+), 124 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberThenable.js b/packages/react-reconciler/src/ReactFiberThenable.js index e34d9ad5e227..956f81e89270 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.js +++ b/packages/react-reconciler/src/ReactFiberThenable.js @@ -212,19 +212,19 @@ export function trackUsedThenable( } }, ); - } - // 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); - const rejectedError = rejectedThenable.reason; - checkIfUseWrappedInAsyncCatch(rejectedError); - throw rejectedError; + // 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); + const rejectedError = rejectedThenable.reason; + checkIfUseWrappedInAsyncCatch(rejectedError); + throw rejectedError; + } } } diff --git a/packages/react/src/ReactChildren.js b/packages/react/src/ReactChildren.js index d671c1503327..d8d2f6707a95 100644 --- a/packages/react/src/ReactChildren.js +++ b/packages/react/src/ReactChildren.js @@ -7,13 +7,7 @@ * @flow */ -import type { - ReactNodeList, - Thenable, - PendingThenable, - FulfilledThenable, - RejectedThenable, -} from 'shared/ReactTypes'; +import type {ReactNodeList} from 'shared/ReactTypes'; import isArray from 'shared/isArray'; import { @@ -81,68 +75,6 @@ function getElementKey(element: any, index: number): string { return index.toString(36); } -function noop() {} - -function resolveThenable(thenable: Thenable): T { - switch (thenable.status) { - case 'fulfilled': { - const fulfilledValue: T = thenable.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError = thenable.reason; - throw rejectedError; - } - default: { - if (typeof thenable.status === 'string') { - // Only instrument the thenable if the status if not defined. If - // it's defined, but an unknown value, assume it's been instrumented by - // some custom userspace implementation. We treat it as "pending". - // Attach a dummy listener, to ensure that any lazy initialization can - // happen. Flight lazily parses JSON when the value is actually awaited. - thenable.then(noop, noop); - } else { - // This is an uncached thenable that we haven't seen before. - - // TODO: Detect infinite ping loops caused by uncached promises. - - const pendingThenable: PendingThenable = (thenable: any); - pendingThenable.status = 'pending'; - pendingThenable.then( - fulfilledValue => { - if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); - fulfilledThenable.status = 'fulfilled'; - fulfilledThenable.value = fulfilledValue; - } - }, - (error: mixed) => { - if (thenable.status === 'pending') { - 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); - const rejectedError = rejectedThenable.reason; - throw rejectedError; - } - } - } - } - throw thenable; -} - function mapIntoArray( children: ?ReactNodeList, array: Array, @@ -175,14 +107,9 @@ function mapIntoArray( invokeCallback = true; break; case REACT_LAZY_TYPE: - const payload = (children: any)._payload; - const init = (children: any)._init; - return mapIntoArray( - init(payload), - array, - escapedPrefix, - nameSoFar, - callback, + throw new Error( + 'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' + + 'We recommend not iterating over children and just rendering them plain.', ); } } @@ -285,19 +212,16 @@ function mapIntoArray( ); } } else if (type === 'object') { + // eslint-disable-next-line react-internal/safe-string-coercion + const childrenString = String((children: any)); + if (typeof (children: any).then === 'function') { - return mapIntoArray( - resolveThenable((children: any)), - array, - escapedPrefix, - nameSoFar, - callback, + throw new Error( + 'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' + + 'We recommend not iterating over children and just rendering them plain.', ); } - // eslint-disable-next-line react-internal/safe-string-coercion - const childrenString = String((children: any)); - throw new Error( `Objects are not valid as a React child (found: ${ childrenString === '[object Object]' diff --git a/packages/react/src/__tests__/ReactChildren-test.js b/packages/react/src/__tests__/ReactChildren-test.js index 92e7ba435a1d..0c405f91b15f 100644 --- a/packages/react/src/__tests__/ReactChildren-test.js +++ b/packages/react/src/__tests__/ReactChildren-test.js @@ -952,36 +952,26 @@ describe('ReactChildren', () => { ); }); - it('should render React.lazy after suspending', async () => { + it('should throw on React.lazy', async () => { const lazyElement = React.lazy(async () => ({default:
})); - function Component() { - return React.Children.map([lazyElement], c => - React.cloneElement(c, {children: 'hi'}), - ); - } - const container = document.createElement('div'); - const root = ReactDOMClient.createRoot(container); - await act(() => { - root.render(); - }); - - expect(container.innerHTML).toBe('
hi
'); + await expect(() => { + React.Children.forEach([lazyElement], () => {}, null); + }).toThrowError( + 'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' + + 'We recommend not iterating over children and just rendering them plain.', + {withoutStack: true}, // There's nothing on the stack + ); }); - it('should render cached Promises after suspending', async () => { + it('should throw on Promises', async () => { const promise = Promise.resolve(
); - function Component() { - return React.Children.map([promise], c => - React.cloneElement(c, {children: 'hi'}), - ); - } - const container = document.createElement('div'); - const root = ReactDOMClient.createRoot(container); - await act(() => { - root.render(); - }); - - expect(container.innerHTML).toBe('
hi
'); + await expect(() => { + React.Children.forEach([promise], () => {}, null); + }).toThrowError( + 'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' + + 'We recommend not iterating over children and just rendering them plain.', + {withoutStack: true}, // There's nothing on the stack + ); }); it('should throw on regex', () => { From 4b32fda0bc56737d5cad451c3a82e9c75202ff90 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Mon, 8 Apr 2024 13:19:39 +0200 Subject: [PATCH 3/3] Revert "Revert "Suspend Thenable/Lazy if it's used in React.Children and unwrap (#28284)"" This reverts commit 74b1848d1d33ca82fa75b43910cb3071b7923b7a. --- .../src/ReactFiberThenable.js | 24 ++--- packages/react/src/ReactChildren.js | 96 +++++++++++++++++-- .../react/src/__tests__/ReactChildren-test.js | 42 ++++---- 3 files changed, 124 insertions(+), 38 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberThenable.js b/packages/react-reconciler/src/ReactFiberThenable.js index 956f81e89270..e34d9ad5e227 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.js +++ b/packages/react-reconciler/src/ReactFiberThenable.js @@ -212,19 +212,19 @@ export function trackUsedThenable( } }, ); + } - // 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); - const rejectedError = rejectedThenable.reason; - checkIfUseWrappedInAsyncCatch(rejectedError); - throw rejectedError; - } + // 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); + const rejectedError = rejectedThenable.reason; + checkIfUseWrappedInAsyncCatch(rejectedError); + throw rejectedError; } } diff --git a/packages/react/src/ReactChildren.js b/packages/react/src/ReactChildren.js index d8d2f6707a95..d671c1503327 100644 --- a/packages/react/src/ReactChildren.js +++ b/packages/react/src/ReactChildren.js @@ -7,7 +7,13 @@ * @flow */ -import type {ReactNodeList} from 'shared/ReactTypes'; +import type { + ReactNodeList, + Thenable, + PendingThenable, + FulfilledThenable, + RejectedThenable, +} from 'shared/ReactTypes'; import isArray from 'shared/isArray'; import { @@ -75,6 +81,68 @@ function getElementKey(element: any, index: number): string { return index.toString(36); } +function noop() {} + +function resolveThenable(thenable: Thenable): T { + switch (thenable.status) { + case 'fulfilled': { + const fulfilledValue: T = thenable.value; + return fulfilledValue; + } + case 'rejected': { + const rejectedError = thenable.reason; + throw rejectedError; + } + default: { + if (typeof thenable.status === 'string') { + // Only instrument the thenable if the status if not defined. If + // it's defined, but an unknown value, assume it's been instrumented by + // some custom userspace implementation. We treat it as "pending". + // Attach a dummy listener, to ensure that any lazy initialization can + // happen. Flight lazily parses JSON when the value is actually awaited. + thenable.then(noop, noop); + } else { + // This is an uncached thenable that we haven't seen before. + + // TODO: Detect infinite ping loops caused by uncached promises. + + const pendingThenable: PendingThenable = (thenable: any); + pendingThenable.status = 'pending'; + pendingThenable.then( + fulfilledValue => { + if (thenable.status === 'pending') { + const fulfilledThenable: FulfilledThenable = (thenable: any); + fulfilledThenable.status = 'fulfilled'; + fulfilledThenable.value = fulfilledValue; + } + }, + (error: mixed) => { + if (thenable.status === 'pending') { + 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); + const rejectedError = rejectedThenable.reason; + throw rejectedError; + } + } + } + } + throw thenable; +} + function mapIntoArray( children: ?ReactNodeList, array: Array, @@ -107,9 +175,14 @@ function mapIntoArray( invokeCallback = true; break; case REACT_LAZY_TYPE: - throw new Error( - 'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' + - 'We recommend not iterating over children and just rendering them plain.', + const payload = (children: any)._payload; + const init = (children: any)._init; + return mapIntoArray( + init(payload), + array, + escapedPrefix, + nameSoFar, + callback, ); } } @@ -212,16 +285,19 @@ function mapIntoArray( ); } } else if (type === 'object') { - // eslint-disable-next-line react-internal/safe-string-coercion - const childrenString = String((children: any)); - if (typeof (children: any).then === 'function') { - throw new Error( - 'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' + - 'We recommend not iterating over children and just rendering them plain.', + return mapIntoArray( + resolveThenable((children: any)), + array, + escapedPrefix, + nameSoFar, + callback, ); } + // eslint-disable-next-line react-internal/safe-string-coercion + const childrenString = String((children: any)); + throw new Error( `Objects are not valid as a React child (found: ${ childrenString === '[object Object]' diff --git a/packages/react/src/__tests__/ReactChildren-test.js b/packages/react/src/__tests__/ReactChildren-test.js index 0c405f91b15f..92e7ba435a1d 100644 --- a/packages/react/src/__tests__/ReactChildren-test.js +++ b/packages/react/src/__tests__/ReactChildren-test.js @@ -952,26 +952,36 @@ describe('ReactChildren', () => { ); }); - it('should throw on React.lazy', async () => { + it('should render React.lazy after suspending', async () => { const lazyElement = React.lazy(async () => ({default:
})); - await expect(() => { - React.Children.forEach([lazyElement], () => {}, null); - }).toThrowError( - 'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' + - 'We recommend not iterating over children and just rendering them plain.', - {withoutStack: true}, // There's nothing on the stack - ); + function Component() { + return React.Children.map([lazyElement], c => + React.cloneElement(c, {children: 'hi'}), + ); + } + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + + expect(container.innerHTML).toBe('
hi
'); }); - it('should throw on Promises', async () => { + it('should render cached Promises after suspending', async () => { const promise = Promise.resolve(
); - await expect(() => { - React.Children.forEach([promise], () => {}, null); - }).toThrowError( - 'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' + - 'We recommend not iterating over children and just rendering them plain.', - {withoutStack: true}, // There's nothing on the stack - ); + function Component() { + return React.Children.map([promise], c => + React.cloneElement(c, {children: 'hi'}), + ); + } + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + + expect(container.innerHTML).toBe('
hi
'); }); it('should throw on regex', () => {