Skip to content
Prev Previous commit
Next Next commit
Don't validate keys in createElement/JSX
We have the same validation in the reconciler and with enableOwnerStacks
we'll have the ability to provide a stack trace with the callsite of the
element.

The way the validation works is that if something is passed through a valid
static slot, an element gets marked as valid. If it has been logged as errored
before, it gets logged as valid so that future logs don't happen.

This change logs for strictly fewer cases - specifically if an element doesn't
get rendered/mounted at all. I updated some tests to start rendering where it
didn't before.
  • Loading branch information
sebmarkbage committed May 23, 2024
commit 90971dc1b01f55626fa062e68f295d9745c54f36
3 changes: 3 additions & 0 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,9 @@ describe('ReactFlight', () => {
'\n' +
'Check the render method of `Component`. See https://react.dev/link/warning-keys for more information.\n' +
' in span (at **)\n' +
// TODO: Because this validates after the div has been mounted, it is part of
// the parent stack but since owner stacks will switch to owners this goes away again.
(gate(flags => flags.enableOwnerStacks) ? ' in div (at **)\n' : '') +
' in Component (at **)\n' +
' in Indirection (at **)\n' +
' in App (at **)',
Expand Down
55 changes: 49 additions & 6 deletions packages/react-reconciler/src/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,61 @@ if (__DEV__) {
// $FlowFixMe[cannot-write] unable to narrow type from mixed to writable object
child._store.validated = true;

const componentName = getComponentNameFromFiber(returnFiber) || 'Component';
const componentName = getComponentNameFromFiber(returnFiber);

if (ownerHasKeyUseWarning[componentName]) {
const componentKey = componentName || 'null';
if (ownerHasKeyUseWarning[componentKey]) {
return;
}
ownerHasKeyUseWarning[componentName] = true;
ownerHasKeyUseWarning[componentKey] = true;

const childOwner = child._owner;
const parentOwner = returnFiber._debugOwner;

let currentComponentErrorInfo = '';
if (parentOwner && typeof parentOwner.tag === 'number') {
const name = getComponentNameFromFiber((parentOwner: any));
if (name) {
currentComponentErrorInfo =
'\n\nCheck the render method of `' + name + '`.';
}
}
if (!currentComponentErrorInfo) {
if (componentName) {
currentComponentErrorInfo = `\n\nCheck the top-level render call using <${componentName}>.`;
}
}

// Usually the current owner is the offender, but if it accepts children as a
// property, it may be the creator of the child that's responsible for
// assigning it a key.
let childOwnerAppendix = '';
if (childOwner != null && parentOwner !== childOwner) {
let ownerName = null;
if (typeof childOwner.tag === 'number') {
ownerName = getComponentNameFromFiber((childOwner: any));
} else if (typeof childOwner.name === 'string') {
ownerName = childOwner.name;
}
if (ownerName) {
// Give the component that originally created this child.
childOwnerAppendix = ` It was passed a child from ${ownerName}.`;
}
}

// We create a fake Fiber for the child to log the stack trace from.
const fiber = createFiberFromElement((child: any), returnFiber.mode, 0);
fiber.return = returnFiber;

const prevDebugFiber = getCurrentDebugFiberInDEV();
setCurrentDebugFiberInDEV(fiber);
console.error(
'Each child in a list should have a unique ' +
'"key" prop. See https://react.dev/link/warning-keys for ' +
'more information.',
'Each child in a list should have a unique "key" prop.' +
'%s%s See https://react.dev/link/warning-keys for more information.',
currentComponentErrorInfo,
childOwnerAppendix,
);
setCurrentDebugFiberInDEV(prevDebugFiber);
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,13 @@ describe('ReactLazy', () => {
await expect(async () => {
await act(() => resolveFakeImport(Foo));
assertLog(['A', 'B']);
}).toErrorDev(' in Text (at **)\n' + ' in Foo (at **)');
}).toErrorDev(
' in Text (at **)\n' +
// TODO: Because this validates after the div has been mounted, it is part of
// the parent stack but since owner stacks will switch to owners this goes away again.
(gate(flags => flags.enableOwnerStacks) ? ' in div (at **)\n' : '') +
' in Foo (at **)',
);
expect(root).toMatchRenderedOutput(<div>AB</div>);
});

Expand Down
24 changes: 19 additions & 5 deletions packages/react-reconciler/src/__tests__/ReactMemo-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,9 @@ describe('memo', () => {
await expect(async () => {
await waitForAll([]);
}).toErrorDev(
'Each child in a list should have a unique "key" prop. See https://react.dev/link/warning-keys for more information.\n' +
'Each child in a list should have a unique "key" prop. ' +
'See https://react.dev/link/warning-keys for more information.\n' +
' in span (at **)\n' +
' in p (at **)',
);
});
Expand All @@ -616,7 +618,10 @@ describe('memo', () => {
await expect(async () => {
await waitForAll([]);
}).toErrorDev(
'Each child in a list should have a unique "key" prop. See https://react.dev/link/warning-keys for more information.\n' +
'Each child in a list should have a unique "key" prop.' +
'\n\nCheck the top-level render call using <Inner>. It was passed a child from Inner. ' +
'See https://react.dev/link/warning-keys for more information.\n' +
' in span (at **)\n' +
' in Inner (at **)\n' +
' in p (at **)',
);
Expand All @@ -636,7 +641,10 @@ describe('memo', () => {
await expect(async () => {
await waitForAll([]);
}).toErrorDev(
'Each child in a list should have a unique "key" prop. See https://react.dev/link/warning-keys for more information.\n' +
'Each child in a list should have a unique "key" prop.' +
'\n\nCheck the top-level render call using <Inner>. It was passed a child from Inner. ' +
'See https://react.dev/link/warning-keys for more information.\n' +
' in span (at **)\n' +
' in Inner (at **)\n' +
' in p (at **)',
);
Expand All @@ -655,7 +663,10 @@ describe('memo', () => {
await expect(async () => {
await waitForAll([]);
}).toErrorDev(
'Each child in a list should have a unique "key" prop. See https://react.dev/link/warning-keys for more information.\n' +
'Each child in a list should have a unique "key" prop.' +
'\n\nCheck the top-level render call using <Outer>. It was passed a child from Outer. ' +
'See https://react.dev/link/warning-keys for more information.\n' +
' in span (at **)\n' +
' in Outer (at **)\n' +
' in p (at **)',
);
Expand All @@ -676,7 +687,10 @@ describe('memo', () => {
await expect(async () => {
await waitForAll([]);
}).toErrorDev(
'Each child in a list should have a unique "key" prop. See https://react.dev/link/warning-keys for more information.\n' +
'Each child in a list should have a unique "key" prop.' +
'\n\nCheck the top-level render call using <Inner>. It was passed a child from Inner. ' +
'See https://react.dev/link/warning-keys for more information.\n' +
' in span (at **)\n' +
' in Inner (at **)\n' +
' in p (at **)',
);
Expand Down
9 changes: 6 additions & 3 deletions packages/react/src/__tests__/ReactChildren-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1015,7 +1015,9 @@ describe('ReactChildren', () => {
}).toErrorDev(
'Warning: ' +
'Each child in a list should have a unique "key" prop.' +
' See https://react.dev/link/warning-keys for more information.' +
'\n\nCheck the top-level render call using <ComponentReturningArray>. It was passed a child from ComponentReturningArray. ' +
'See https://react.dev/link/warning-keys for more information.' +
'\n in div (at **)' +
'\n in ComponentReturningArray (at **)',
);
});
Expand Down Expand Up @@ -1044,8 +1046,9 @@ describe('ReactChildren', () => {
}).toErrorDev(
'Warning: ' +
'Each child in a list should have a unique "key" prop.' +
' See https://react.dev/link/warning-keys for more information.',
{withoutStack: true}, // There's nothing on the stack
'\n\nCheck the top-level render call using <Root>. ' +
'See https://react.dev/link/warning-keys for more information.' +
'\n in div (at **)',
);
});
});
Expand Down
27 changes: 19 additions & 8 deletions packages/react/src/__tests__/ReactElementClone-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -364,18 +364,29 @@ describe('ReactElementClone', () => {
expect(cloneInstance4.props.prop).toBe('newTestKey');
});

it('warns for keys for arrays of elements in rest args', () => {
expect(() =>
React.cloneElement(<div />, null, [<div />, <div />]),
).toErrorDev('Each child in a list should have a unique "key" prop.');
it('warns for keys for arrays of elements in rest args', async () => {
const root = ReactDOMClient.createRoot(document.createElement('div'));
await expect(async () => {
await act(() => {
root.render(React.cloneElement(<div />, null, [<div />, <div />]));
});
}).toErrorDev('Each child in a list should have a unique "key" prop.');
});

it('does not warns for arrays of elements with keys', () => {
React.cloneElement(<div />, null, [<div key="#1" />, <div key="#2" />]);
it('does not warns for arrays of elements with keys', async () => {
const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => {
root.render(
React.cloneElement(<div />, null, [<div key="#1" />, <div key="#2" />]),
);
});
});

it('does not warn when the element is directly in rest args', () => {
React.cloneElement(<div />, null, <div />, <div />);
it('does not warn when the element is directly in rest args', async () => {
const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => {
root.render(React.cloneElement(<div />, null, <div />, <div />));
});
});

it('does not warn when the array contains a non-element', () => {
Expand Down
73 changes: 55 additions & 18 deletions packages/react/src/__tests__/ReactElementValidator-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,22 @@ describe('ReactElementValidator', () => {
act = require('internal-test-utils').act;
ComponentClass = class extends React.Component {
render() {
return React.createElement('div');
return React.createElement('div', null, this.props.children);
}
};
});

it('warns for keys for arrays of elements in rest args', () => {
expect(() => {
React.createElement(ComponentClass, null, [
React.createElement(ComponentClass),
React.createElement(ComponentClass),
]);
it('warns for keys for arrays of elements in rest args', async () => {
const root = ReactDOMClient.createRoot(document.createElement('div'));
await expect(async () => {
await act(() =>
root.render(
React.createElement(ComponentClass, null, [
React.createElement(ComponentClass),
React.createElement(ComponentClass),
]),
),
);
}).toErrorDev('Each child in a list should have a unique "key" prop.');
});

Expand All @@ -67,14 +72,18 @@ describe('ReactElementValidator', () => {
await act(() => root.render(React.createElement(ComponentWrapper)));
}).toErrorDev(
'Each child in a list should have a unique "key" prop.' +
'\n\nCheck the render method of `InnerClass`. ' +
'\n\nCheck the render method of `' +
(gate(flags => flags.enableOwnerStacks)
? 'ComponentClass'
: 'InnerClass') +
'`. ' +
'It was passed a child from ComponentWrapper. ',
);
});

it('warns for keys for arrays with no owner or parent info', async () => {
function Anonymous() {
return <div />;
function Anonymous({children}) {
return <div>{children}</div>;
}
Object.defineProperty(Anonymous, 'name', {value: undefined});

Expand All @@ -84,9 +93,16 @@ describe('ReactElementValidator', () => {
const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => root.render(<Anonymous>{divs}</Anonymous>));
}).toErrorDev(
'Warning: Each child in a list should have a unique ' +
'"key" prop. See https://react.dev/link/warning-keys for more information.\n' +
' in div (at **)',
gate(flags => flags.enableOwnerStacks)
? // For owner stacks the parent being validated is the div.
'Warning: Each child in a list should have a unique ' +
'"key" prop.' +
'\n\nCheck the top-level render call using <div>. ' +
'See https://react.dev/link/warning-keys for more information.\n' +
' in div (at **)'
: 'Warning: Each child in a list should have a unique ' +
'"key" prop. See https://react.dev/link/warning-keys for more information.\n' +
' in div (at **)',
);
});

Expand Down Expand Up @@ -126,6 +142,9 @@ describe('ReactElementValidator', () => {
'"key" prop.\n\nCheck the render method of `Component`. See ' +
'https://react.dev/link/warning-keys for more information.\n' +
' in div (at **)\n' +
// TODO: Because this validates after the div has been mounted, it is part of
// the parent stack but since owner stacks will switch to owners this goes away again.
(gate(flags => flags.enableOwnerStacks) ? ' in div (at **)\n' : '') +
' in Component (at **)\n' +
' in Parent (at **)\n' +
' in GrandParent (at **)',
Expand Down Expand Up @@ -153,7 +172,7 @@ describe('ReactElementValidator', () => {
);
});

it('warns for keys for iterables of elements in rest args', () => {
it('warns for keys for iterables of elements in rest args', async () => {
const iterable = {
'@@iterator': function () {
let i = 0;
Expand All @@ -169,9 +188,23 @@ describe('ReactElementValidator', () => {
},
};

expect(() =>
React.createElement(ComponentClass, null, iterable),
).toErrorDev('Each child in a list should have a unique "key" prop.');
await expect(async () => {
const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() =>
root.render(React.createElement(ComponentClass, null, iterable)),
);
}).toErrorDev(
gate(flag => flag.enableOwnerStacks)
? 'Each child in a list should have a unique "key" prop.'
: // Since each pass generates a new element, it doesn't get marked as
// validated and it gets rechecked each time.

[
'Each child in a list should have a unique "key" prop.',
'Each child in a list should have a unique "key" prop.',
'Each child in a list should have a unique "key" prop.',
],
);
});

it('does not warns for arrays of elements with keys', () => {
Expand Down Expand Up @@ -226,8 +259,12 @@ describe('ReactElementValidator', () => {
const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => root.render(React.createElement(ParentComp)));
}).toErrorDev(
'Each child in a list should have a unique "key" prop. ' +
'Each child in a list should have a unique "key" prop.' +
'\n\nCheck the render method of `ParentComp`. It was passed a child from MyComp. ' +
'See https://react.dev/link/warning-keys for more information.\n' +
// TODO: Because this validates after the div has been mounted, it is part of
// the parent stack but since owner stacks will switch to owners this goes away again.
' in div (at **)\n' +
' in MyComp (at **)\n' +
' in ParentComp (at **)',
);
Expand Down
Loading