Skip to content

Commit

Permalink
Validate key in Flight
Browse files Browse the repository at this point in the history 
Elements that render Server Components are validated inside Flight.
Others pass the validated flag to the client.
  • Loading branch information
@sebmarkbage
sebmarkbage committed May 16, 2024
1 parent 3bc8280 commit c8e1421
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 21 deletions.
6 changes: 4 additions & 2 deletions packages/react-client/src/ReactFlightClient.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,7 @@ function createElement(
props: mixed,
owner: null | ReactComponentInfo, // DEV-only
stack: null | string, // DEV-only
validated: number, // DEV-only
): React$Element<any> {
let element: any;
if (__DEV__ && enableRefAsProp) {
Expand Down Expand Up @@ -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', {
Expand All @@ -630,7 +631,7 @@ function createElement(
configurable: false,
enumerable: false,
writable: true,
value: {stack: stack},
value: stack,
});
Object.defineProperty(element, '_debugTask', {
configurable: false,
Expand Down Expand Up @@ -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;
Expand Down
37 changes: 29 additions & 8 deletions packages/react-client/src/__tests__/ReactFlight-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div key="this has a key but parent doesn't" />;
}
expect(() => {
const transport = ReactNoopFlightServer.render(
<div>{Array(6).fill(<NoKey />)}</div>,
);
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(
<Parent>{Array(6).fill(<div>no key</div>)}</Parent>,
);
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 <ParentClient>. ' +
'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.',
);
});

Expand Down Expand Up @@ -2309,7 +2330,7 @@ describe('ReactFlight', () => {
}

function ThirdPartyFragmentComponent() {
return [<span>Who</span>, ' ', <span>dis?</span>];
return [<span key="1">Who</span>, ' ', <span key="2">dis?</span>];
}

function ServerComponent({transport}) {
Expand All @@ -2321,7 +2342,7 @@ describe('ReactFlight', () => {
const promiseComponent = Promise.resolve(<ThirdPartyComponent />);

const thirdPartyTransport = ReactNoopFlightServer.render(
[promiseComponent, lazy, <ThirdPartyFragmentComponent />],
[promiseComponent, lazy, <ThirdPartyFragmentComponent key="3" />],
{
environmentName: 'third-party',
},
Expand Down Expand Up @@ -2413,8 +2434,8 @@ describe('ReactFlight', () => {
const iteratorPromise = new Promise(r => (resolve = r));

async function* ThirdPartyAsyncIterableComponent({item, initial}) {
yield <span>Who</span>;
yield <span>dis?</span>;
yield <span key="1">Who</span>;
yield <span key="2">dis?</span>;
resolve();
}

Expand Down
15 changes: 10 additions & 5 deletions packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<>
Expand All @@ -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 () => {
Expand Down
103 changes: 97 additions & 6 deletions packages/react-server/src/ReactFlightServer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ export type Request = {
onPostpone: (reason: string) => void,
// DEV-only
environmentName: string,
didWarnForKey: null | WeakSet<ReactComponentInfo>,
};

const {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -965,6 +967,7 @@ function renderFunctionComponent<Props>(
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
Expand Down Expand Up @@ -1005,6 +1008,10 @@ function renderFunctionComponent<Props>(
// 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);
Expand Down Expand Up @@ -1102,6 +1109,8 @@ function renderFunctionComponent<Props>(
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..
Expand All @@ -1124,11 +1133,68 @@ function renderFunctionComponent<Props>(
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<ReactClientValue>,
): 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.
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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
Expand All @@ -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: {
Expand All @@ -1372,6 +1457,7 @@ function renderElement(
props,
owner,
stack,
validated,
);
}
case REACT_FORWARD_REF_TYPE: {
Expand All @@ -1383,6 +1469,7 @@ function renderElement(
props,
owner,
stack,
validated,
);
}
case REACT_MEMO_TYPE: {
Expand All @@ -1395,6 +1482,7 @@ function renderElement(
props,
owner,
stack,
validated,
);
}
}
Expand Down Expand Up @@ -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: {
Expand Down

0 comments on commit c8e1421

Please sign in to comment.