From 5a71659c4318481a3b06e6a5e239d2d61eb7fe81 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. --- .../react-client/src/ReactFlightClient.js | 6 +- .../src/__tests__/ReactFlight-test.js | 37 +++++-- .../__tests__/ReactFlightDOMBrowser-test.js | 15 ++- .../react-server/src/ReactFlightServer.js | 103 +++++++++++++++++- 4 files changed, 140 insertions(+), 21 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index c64376c1f0a82..c7d7f61ad5e79 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, @@ -1034,6 +1035,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 55ccb44fb041c..b234e7cda8f51 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -596,6 +596,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( <> @@ -606,11 +610,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/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 650b448b80e4c..31c7e4c3b9218 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); @@ -1102,6 +1109,8 @@ function renderFunctionComponent( if (__DEV__) { (result: any)._debugInfo = iterableChild._debugInfo; } + } else if (__DEV__ && (result: any).$$typeof === REACT_ELEMENT_TYPE) { + (result: any)._store.validated = 1; } } // Track this element's key on the Server Component on the keyPath context.. @@ -1124,11 +1133,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 +1297,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 +1309,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 +1359,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 +1380,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 +1399,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 +1424,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 +1457,7 @@ function renderElement( props, owner, stack, + validated, ); } case REACT_FORWARD_REF_TYPE: { @@ -1383,6 +1469,7 @@ function renderElement( props, owner, stack, + validated, ); } case REACT_MEMO_TYPE: { @@ -1395,6 +1482,7 @@ function renderElement( props, owner, stack, + validated, ); } } @@ -1963,8 +2051,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: {