Skip to content

[Flight] Warn for keyless fragments in an array #30588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 3, 2024
Merged

[Flight] Warn for keyless fragments in an array #30588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/react-client/src/__tests__/ReactFlight-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1632,6 +1632,25 @@ describe('ReactFlight', () => {
}).toErrorDev('Each child in a list should have a unique "key" prop.');
});

// @gate !__DEV__ || enableOwnerStacks
it('should warn in DEV a child is missing keys on a fragment', () => {
expect(() => {
// While we're on the server we need to have the Server version active to track component stacks.
jest.resetModules();
jest.mock('react', () => ReactServer);
const transport = ReactNoopFlightServer.render(
ReactServer.createElement(
'div',
null,
Array(6).fill(ReactServer.createElement(ReactServer.Fragment)),
),
);
jest.resetModules();
jest.mock('react', () => React);
ReactNoopFlightClient.read(transport);
}).toErrorDev('Each child in a list should have a unique "key" prop.');
});

it('should warn in DEV a child is missing keys in client component', async () => {
function ParentClient({children}) {
return children;
Expand Down
40 changes: 22 additions & 18 deletions packages/react-server/src/ReactFlightServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,7 +1029,7 @@ function renderFunctionComponent<Props>(
const componentDebugID = debugID;
const componentName =
(Component: any).displayName || Component.name || '';
const componentEnv = request.environmentName();
const componentEnv = (0, request.environmentName)();
request.pendingChunks++;
componentDebugInfo = ({
name: componentName,
Expand All @@ -1056,14 +1056,8 @@ function renderFunctionComponent<Props>(
// We've emitted the latest environment for this task so we track that.
task.environmentName = componentEnv;

if (enableOwnerStacks) {
warnForMissingKey(
request,
key,
validated,
componentDebugInfo,
task.debugTask,
);
if (enableOwnerStacks && validated === 2) {
warnForMissingKey(request, key, componentDebugInfo, task.debugTask);
}
}
prepareToUseHooksForComponent(prevThenableState, componentDebugInfo);
Expand Down Expand Up @@ -1256,15 +1250,10 @@ function renderFunctionComponent<Props>(
function warnForMissingKey(
request: Request,
key: null | string,
validated: number,
componentDebugInfo: ReactComponentInfo,
debugTask: null | ConsoleTask,
): void {
if (__DEV__) {
if (validated !== 2) {
return;
}

let didWarnForKey = request.didWarnForKey;
if (didWarnForKey == null) {
didWarnForKey = request.didWarnForKey = new WeakSet();
Expand Down Expand Up @@ -1573,6 +1562,21 @@ function renderElement(
} else if (type === REACT_FRAGMENT_TYPE && key === null) {
// For key-less fragments, we add a small optimization to avoid serializing
// it as a wrapper.
if (__DEV__ && enableOwnerStacks && validated === 2) {
// Create a fake owner node for the error stack.
const componentDebugInfo: ReactComponentInfo = {
name: 'Fragment',
env: (0, request.environmentName)(),
owner: task.debugOwner,
stack:
task.debugStack === null
? null
: filterStackTrace(request, task.debugStack, 1),
debugStack: task.debugStack,
debugTask: task.debugTask,
};
warnForMissingKey(request, key, componentDebugInfo, task.debugTask);
}
const prevImplicitSlot = task.implicitSlot;
if (task.keyPath === null) {
task.implicitSlot = true;
Expand Down Expand Up @@ -2921,7 +2925,7 @@ function emitErrorChunk(
if (__DEV__) {
let message;
let stack: ReactStackTrace;
let env = request.environmentName();
let env = (0, request.environmentName)();
try {
if (error instanceof Error) {
// eslint-disable-next-line react-internal/safe-string-coercion
Expand Down Expand Up @@ -3442,7 +3446,7 @@ function emitConsoleChunk(
}

// TODO: Don't double badge if this log came from another Flight Client.
const env = request.environmentName();
const env = (0, request.environmentName)();
const payload = [methodName, stackTrace, owner, env];
// $FlowFixMe[method-unbinding]
payload.push.apply(payload, args);
Expand Down Expand Up @@ -3611,7 +3615,7 @@ function retryTask(request: Request, task: Task): void {
request.writtenObjects.set(resolvedModel, serializeByValueID(task.id));

if (__DEV__) {
const currentEnv = request.environmentName();
const currentEnv = (0, request.environmentName)();
if (currentEnv !== task.environmentName) {
// The environment changed since we last emitted any debug information for this
// task. We emit an entry that just includes the environment name change.
Expand All @@ -3629,7 +3633,7 @@ function retryTask(request: Request, task: Task): void {
const json: string = stringify(resolvedModel);

if (__DEV__) {
const currentEnv = request.environmentName();
const currentEnv = (0, request.environmentName)();
if (currentEnv !== task.environmentName) {
// The environment changed since we last emitted any debug information for this
// task. We emit an entry that just includes the environment name change.
Expand Down
Loading