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.

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.
  • Loading branch information
@sebmarkbage
sebmarkbage committed May 21, 2024
1 parent c62d157 commit 168b54e
Show file tree
Hide file tree
Showing 5 changed files with 230 additions and 23 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 @@ -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;
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 @@ -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(
<>
Expand All @@ -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 () => {
Expand Down
72 changes: 70 additions & 2 deletions packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,41 @@ describe('ReactFlightDOMEdge', () => {
return str;
}
const element = <ServerComponent />;
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();
Expand Down Expand Up @@ -288,7 +322,41 @@ describe('ReactFlightDOMEdge', () => {
return div;
}
const element = <ServerComponent />;
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(
'<div>this is a long return value</div>',
);
Expand Down
Loading

0 comments on commit 168b54e

Please sign in to comment.