From 168b54e4ab338b71c6530c4882e737ee559a2906 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 15 May 2024 21:42:59 -0400 Subject: [PATCH] Validate key in Flight Elements that render Server Components are validated inside Flight. Others pass the validated flag to the client. Instead of running validation in certain cases (block list) we're marking all the cases that don't need keys (allow list) which is tricky since we need to cover all cases. This might lead to false warnings. --- .../react-client/src/ReactFlightClient.js | 6 +- .../src/__tests__/ReactFlight-test.js | 37 ++++-- .../__tests__/ReactFlightDOMBrowser-test.js | 15 ++- .../src/__tests__/ReactFlightDOMEdge-test.js | 72 +++++++++- .../react-server/src/ReactFlightServer.js | 123 +++++++++++++++++- 5 files changed, 230 insertions(+), 23 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index a805bf7c9238c..0c6902d85be36 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -565,6 +565,7 @@ function createElement( props: mixed, owner: null | ReactComponentInfo, // DEV-only stack: null | string, // DEV-only + validated: number, // DEV-only ): React$Element { let element: any; if (__DEV__ && enableRefAsProp) { @@ -616,7 +617,7 @@ function createElement( configurable: false, enumerable: false, writable: true, - value: 1, // This element has already been validated on the server. + value: enableOwnerStacks ? validated : 1, // Whether the element has already been validated on the server. }); // debugInfo contains Server Component debug information. Object.defineProperty(element, '_debugInfo', { @@ -630,7 +631,7 @@ function createElement( configurable: false, enumerable: false, writable: true, - value: {stack: stack}, + value: stack, }); Object.defineProperty(element, '_debugTask', { configurable: false, @@ -1039,6 +1040,7 @@ function parseModelTuple( tuple[3], __DEV__ ? (tuple: any)[4] : null, __DEV__ && enableOwnerStacks ? (tuple: any)[5] : null, + __DEV__ && enableOwnerStacks ? (tuple: any)[6] : 0, ); } return value; diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 672218443b51e..b7a73eb2ab57e 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1389,19 +1389,40 @@ describe('ReactFlight', () => { ReactNoopFlightClient.read(transport); }); - it('should warn in DEV a child is missing keys', () => { + it('should warn in DEV a child is missing keys on server component', () => { + function NoKey({children}) { + return
; + } + expect(() => { + const transport = ReactNoopFlightServer.render( +
{Array(6).fill()}
, + ); + ReactNoopFlightClient.read(transport); + }).toErrorDev('Each child in a list should have a unique "key" prop.', { + withoutStack: gate(flags => flags.enableOwnerStacks), + }); + }); + + it('should warn in DEV a child is missing keys in client component', async () => { function ParentClient({children}) { return children; } const Parent = clientReference(ParentClient); - expect(() => { + await expect(async () => { const transport = ReactNoopFlightServer.render( {Array(6).fill(
no key
)}, ); ReactNoopFlightClient.read(transport); + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); }).toErrorDev( - 'Each child in a list should have a unique "key" prop. ' + - 'See https://react.dev/link/warning-keys for more information.', + gate(flags => flags.enableOwnerStacks) + ? 'Each child in a list should have a unique "key" prop.' + + '\n\nCheck the top-level render call using . ' + + 'See https://react.dev/link/warning-keys for more information.' + : 'Each child in a list should have a unique "key" prop. ' + + 'See https://react.dev/link/warning-keys for more information.', ); }); @@ -2309,7 +2330,7 @@ describe('ReactFlight', () => { } function ThirdPartyFragmentComponent() { - return [Who, ' ', dis?]; + return [Who, ' ', dis?]; } function ServerComponent({transport}) { @@ -2321,7 +2342,7 @@ describe('ReactFlight', () => { const promiseComponent = Promise.resolve(); const thirdPartyTransport = ReactNoopFlightServer.render( - [promiseComponent, lazy, ], + [promiseComponent, lazy, ], { environmentName: 'third-party', }, @@ -2413,8 +2434,8 @@ describe('ReactFlight', () => { const iteratorPromise = new Promise(r => (resolve = r)); async function* ThirdPartyAsyncIterableComponent({item, initial}) { - yield Who; - yield dis?; + yield Who; + yield dis?; resolve(); } 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 d0630287dbf8b..0478785f87839 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -645,6 +645,10 @@ describe('ReactFlightDOMBrowser', () => { } const Parent = clientExports(ParentClient); const ParentModule = clientExports({Parent: ParentClient}); + + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + await expect(async () => { const stream = ReactServerDOMServer.renderToReadableStream( <> @@ -655,11 +659,12 @@ describe('ReactFlightDOMBrowser', () => { , webpackMap, ); - await ReactServerDOMClient.createFromReadableStream(stream); - }).toErrorDev( - 'Each child in a list should have a unique "key" prop. ' + - 'See https://react.dev/link/warning-keys for more information.', - ); + const result = + await ReactServerDOMClient.createFromReadableStream(stream); + await act(() => { + root.render(result); + }); + }).toErrorDev('Each child in a list should have a unique "key" prop.'); }); it('basic use(promise)', async () => { diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index cd4201c8ddc9b..90597d106b83e 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -256,7 +256,41 @@ describe('ReactFlightDOMEdge', () => { return str; } const element = ; - const children = new Array(30).fill(element); + // Hardcoded list to avoid the key warning + const children = ( + <> + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + + ); const resolvedChildren = new Array(30).fill(str); const stream = ReactServerDOMServer.renderToReadableStream(children); const [stream1, stream2] = passThrough(stream).tee(); @@ -288,7 +322,41 @@ describe('ReactFlightDOMEdge', () => { return div; } const element = ; - const children = new Array(30).fill(element); + // Hardcoded list to avoid the key warning + const children = ( + <> + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + {element} + + ); const resolvedChildren = new Array(30).fill( '
this is a long return value
', ); diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 650b448b80e4c..59d005df8d93d 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -399,6 +399,7 @@ export type Request = { onPostpone: (reason: string) => void, // DEV-only environmentName: string, + didWarnForKey: null | WeakSet, }; const { @@ -500,6 +501,7 @@ export function createRequest( if (__DEV__) { request.environmentName = environmentName === undefined ? 'Server' : environmentName; + request.didWarnForKey = null; } const rootTask = createTask(request, model, null, false, abortSet); pingedTasks.push(rootTask); @@ -965,6 +967,7 @@ function renderFunctionComponent( props: Props, owner: null | ReactComponentInfo, // DEV-only stack: null | string, // DEV-only + validated: number, // DEV-only ): ReactJSONValue { // Reset the task's thenable state before continuing, so that if a later // component suspends we can reuse the same task object. If the same @@ -1005,6 +1008,10 @@ function renderFunctionComponent( // being no references to this as an owner. outlineModel(request, componentDebugInfo); emitDebugChunk(request, componentDebugID, componentDebugInfo); + + if (enableOwnerStacks) { + warnForMissingKey(request, key, validated, componentDebugInfo); + } } prepareToUseHooksForComponent(prevThenableState, componentDebugInfo); result = callComponentInDEV(Component, props, componentDebugInfo); @@ -1019,6 +1026,23 @@ function renderFunctionComponent( // When the return value is in children position we can resolve it immediately, // to its value without a wrapper if it's synchronously available. const thenable: Thenable = result; + if (__DEV__) { + // If the thenable resolves to an element, then it was in a static position, + // the return value of a Server Component. That doesn't need further validation + // of keys. The Server Component itself would have had a key. + thenable.then( + resolvedValue => { + if ( + typeof resolvedValue === 'object' && + resolvedValue !== null && + resolvedValue.$$typeof === REACT_ELEMENT_TYPE + ) { + resolvedValue._store.validated = 1; + } + }, + () => {}, + ); + } if (thenable.status === 'fulfilled') { return thenable.value; } @@ -1102,6 +1126,11 @@ function renderFunctionComponent( if (__DEV__) { (result: any)._debugInfo = iterableChild._debugInfo; } + } else if (__DEV__ && (result: any).$$typeof === REACT_ELEMENT_TYPE) { + // If the server component renders to an element, then it was in a static position. + // That doesn't need further validation of keys. The Server Component itself would + // have had a key. + (result: any)._store.validated = 1; } } // Track this element's key on the Server Component on the keyPath context.. @@ -1124,11 +1153,68 @@ function renderFunctionComponent( return json; } +function warnForMissingKey( + request: Request, + key: null | string, + validated: number, + componentDebugInfo: ReactComponentInfo, +): void { + if (__DEV__) { + if (validated !== 2) { + return; + } + + let didWarnForKey = request.didWarnForKey; + if (didWarnForKey == null) { + didWarnForKey = request.didWarnForKey = new WeakSet(); + } + const parentOwner = componentDebugInfo.owner; + if (parentOwner != null) { + if (didWarnForKey.has(parentOwner)) { + // We already warned for other children in this parent. + return; + } + didWarnForKey.add(parentOwner); + } + + // Call with the server component as the currently rendering component + // for context. + callComponentInDEV( + () => { + console.error( + 'Each child in a list should have a unique "key" prop.' + + '%s%s See https://react.dev/link/warning-keys for more information.', + '', + '', + ); + }, + null, + componentDebugInfo, + ); + } +} + function renderFragment( request: Request, task: Task, children: $ReadOnlyArray, ): ReactJSONValue { + if (__DEV__) { + for (let i = 0; i < children.length; i++) { + const child = children[i]; + if ( + child !== null && + typeof child === 'object' && + child.$$typeof === REACT_ELEMENT_TYPE + ) { + const element: ReactElement = (child: any); + if (element.key === null && !element._store.validated) { + element._store.validated = 2; + } + } + } + } + if (task.keyPath !== null) { // We have a Server Component that specifies a key but we're now splitting // the tree using a fragment. @@ -1231,6 +1317,7 @@ function renderClientElement( props: any, owner: null | ReactComponentInfo, // DEV-only stack: null | string, // DEV-only + validated: number, // DEV-only ): ReactJSONValue { // We prepend the terminal client element that actually gets serialized with // the keys of any Server Components which are not serialized. @@ -1242,7 +1329,7 @@ function renderClientElement( } const element = __DEV__ ? enableOwnerStacks - ? [REACT_ELEMENT_TYPE, type, key, props, owner, stack] + ? [REACT_ELEMENT_TYPE, type, key, props, owner, stack, validated] : [REACT_ELEMENT_TYPE, type, key, props, owner] : [REACT_ELEMENT_TYPE, type, key, props]; if (task.implicitSlot && key !== null) { @@ -1292,6 +1379,7 @@ function renderElement( props: any, owner: null | ReactComponentInfo, // DEV only stack: null | string, // DEV only + validated: number, // DEV only ): ReactJSONValue { if (ref !== null && ref !== undefined) { // When the ref moves to the regular props object this will implicitly @@ -1312,7 +1400,15 @@ function renderElement( if (typeof type === 'function') { if (isClientReference(type) || isOpaqueTemporaryReference(type)) { // This is a reference to a Client Component. - return renderClientElement(task, type, key, props, owner, stack); + return renderClientElement( + task, + type, + key, + props, + owner, + stack, + validated, + ); } // This is a Server Component. return renderFunctionComponent( @@ -1323,10 +1419,11 @@ function renderElement( props, owner, stack, + validated, ); } else if (typeof type === 'string') { // This is a host element. E.g. HTML. - return renderClientElement(task, type, key, props, owner, stack); + return renderClientElement(task, type, key, props, owner, stack, validated); } else if (typeof type === 'symbol') { if (type === REACT_FRAGMENT_TYPE && key === null) { // For key-less fragments, we add a small optimization to avoid serializing @@ -1347,11 +1444,19 @@ function renderElement( } // This might be a built-in React component. We'll let the client decide. // Any built-in works as long as its props are serializable. - return renderClientElement(task, type, key, props, owner, stack); + return renderClientElement(task, type, key, props, owner, stack, validated); } else if (type != null && typeof type === 'object') { if (isClientReference(type)) { // This is a reference to a Client Component. - return renderClientElement(task, type, key, props, owner, stack); + return renderClientElement( + task, + type, + key, + props, + owner, + stack, + validated, + ); } switch (type.$$typeof) { case REACT_LAZY_TYPE: { @@ -1372,6 +1477,7 @@ function renderElement( props, owner, stack, + validated, ); } case REACT_FORWARD_REF_TYPE: { @@ -1383,6 +1489,7 @@ function renderElement( props, owner, stack, + validated, ); } case REACT_MEMO_TYPE: { @@ -1395,6 +1502,7 @@ function renderElement( props, owner, stack, + validated, ); } } @@ -1963,8 +2071,11 @@ function renderModelDestructive( props, __DEV__ ? element._owner : null, __DEV__ && enableOwnerStacks - ? filterDebugStack(element._debugStack) + ? !element._debugStack || typeof element._debugStack === 'string' + ? element._debugStack + : filterDebugStack(element._debugStack) : null, + __DEV__ && enableOwnerStacks ? element._store.validated : 0, ); } case REACT_LAZY_TYPE: {