Skip to content

Commit 07f46ec

Browse files
sebmarkbagesebmarkbage
authored
Turn on key spread warning in jsx-runtime for everyone (#25697)
This improves the error message a bit and ensures that we recommend putting the key first, not last, which ensures that the faster `jsx-runtime` is used. This only affects the modern "automatic" JSX transform.
1 parent d65b88d commit 07f46ec

11 files changed

+149
-108
lines changed

‎packages/react/src/ReactElementValidator.js‎

Lines changed: 109 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,8 @@ function validateFragmentProps(fragment) {
279279
}
280280
}
281281

282+
const didWarnAboutKeySpread = {};
283+
282284
export function jsxWithValidation(
283285
type,
284286
props,
@@ -287,115 +289,132 @@ export function jsxWithValidation(
287289
source,
288290
self,
289291
) {
290-
const validType = isValidElementType(type);
291-
292-
// We warn in this case but don't throw. We expect the element creation to
293-
// succeed and there will likely be errors in render.
294-
if (!validType) {
295-
let info = '';
296-
if (
297-
type === undefined ||
298-
(typeof type === 'object' &&
299-
type !== null &&
300-
Object.keys(type).length === 0)
301-
) {
302-
info +=
303-
' You likely forgot to export your component from the file ' +
304-
"it's defined in, or you might have mixed up default and named imports.";
305-
}
292+
if (__DEV__) {
293+
const validType = isValidElementType(type);
294+
295+
// We warn in this case but don't throw. We expect the element creation to
296+
// succeed and there will likely be errors in render.
297+
if (!validType) {
298+
let info = '';
299+
if (
300+
type === undefined ||
301+
(typeof type === 'object' &&
302+
type !== null &&
303+
Object.keys(type).length === 0)
304+
) {
305+
info +=
306+
' You likely forgot to export your component from the file ' +
307+
"it's defined in, or you might have mixed up default and named imports.";
308+
}
306309

307-
const sourceInfo = getSourceInfoErrorAddendum(source);
308-
if (sourceInfo) {
309-
info += sourceInfo;
310-
} else {
311-
info += getDeclarationErrorAddendum();
312-
}
310+
const sourceInfo = getSourceInfoErrorAddendum(source);
311+
if (sourceInfo) {
312+
info += sourceInfo;
313+
} else {
314+
info += getDeclarationErrorAddendum();
315+
}
313316

314-
let typeString;
315-
if (type === null) {
316-
typeString = 'null';
317-
} else if (isArray(type)) {
318-
typeString = 'array';
319-
} else if (type !== undefined && type.$$typeof === REACT_ELEMENT_TYPE) {
320-
typeString = `<${getComponentNameFromType(type.type) || 'Unknown'} />`;
321-
info =
322-
' Did you accidentally export a JSX literal instead of a component?';
323-
} else {
324-
typeString = typeof type;
325-
}
317+
let typeString;
318+
if (type === null) {
319+
typeString = 'null';
320+
} else if (isArray(type)) {
321+
typeString = 'array';
322+
} else if (type !== undefined && type.$$typeof === REACT_ELEMENT_TYPE) {
323+
typeString = `<${getComponentNameFromType(type.type) || 'Unknown'} />`;
324+
info =
325+
' Did you accidentally export a JSX literal instead of a component?';
326+
} else {
327+
typeString = typeof type;
328+
}
326329

327-
if (__DEV__) {
328-
console.error(
329-
'React.jsx: type is invalid -- expected a string (for ' +
330-
'built-in components) or a class/function (for composite ' +
331-
'components) but got: %s.%s',
332-
typeString,
333-
info,
334-
);
330+
if (__DEV__) {
331+
console.error(
332+
'React.jsx: type is invalid -- expected a string (for ' +
333+
'built-in components) or a class/function (for composite ' +
334+
'components) but got: %s.%s',
335+
typeString,
336+
info,
337+
);
338+
}
335339
}
336-
}
337340

338-
const element = jsxDEV(type, props, key, source, self);
339-
340-
// The result can be nullish if a mock or a custom function is used.
341-
// TODO: Drop this when these are no longer allowed as the type argument.
342-
if (element == null) {
343-
return element;
344-
}
345-
346-
// Skip key warning if the type isn't valid since our key validation logic
347-
// doesn't expect a non-string/function type and can throw confusing errors.
348-
// We don't want exception behavior to differ between dev and prod.
349-
// (Rendering will throw with a helpful message and as soon as the type is
350-
// fixed, the key warnings will appear.)
341+
const element = jsxDEV(type, props, key, source, self);
351342

352-
if (validType) {
353-
const children = props.children;
354-
if (children !== undefined) {
355-
if (isStaticChildren) {
356-
if (isArray(children)) {
357-
for (let i = 0; i < children.length; i++) {
358-
validateChildKeys(children[i], type);
359-
}
343+
// The result can be nullish if a mock or a custom function is used.
344+
// TODO: Drop this when these are no longer allowed as the type argument.
345+
if (element == null) {
346+
return element;
347+
}
360348

361-
if (Object.freeze) {
362-
Object.freeze(children);
349+
// Skip key warning if the type isn't valid since our key validation logic
350+
// doesn't expect a non-string/function type and can throw confusing errors.
351+
// We don't want exception behavior to differ between dev and prod.
352+
// (Rendering will throw with a helpful message and as soon as the type is
353+
// fixed, the key warnings will appear.)
354+
355+
if (validType) {
356+
const children = props.children;
357+
if (children !== undefined) {
358+
if (isStaticChildren) {
359+
if (isArray(children)) {
360+
for (let i = 0; i < children.length; i++) {
361+
validateChildKeys(children[i], type);
362+
}
363+
364+
if (Object.freeze) {
365+
Object.freeze(children);
366+
}
367+
} else {
368+
if (__DEV__) {
369+
console.error(
370+
'React.jsx: Static children should always be an array. ' +
371+
'You are likely explicitly calling React.jsxs or React.jsxDEV. ' +
372+
'Use the Babel transform instead.',
373+
);
374+
}
363375
}
364376
} else {
365-
if (__DEV__) {
366-
console.error(
367-
'React.jsx: Static children should always be an array. ' +
368-
'You are likely explicitly calling React.jsxs or React.jsxDEV. ' +
369-
'Use the Babel transform instead.',
370-
);
371-
}
377+
validateChildKeys(children, type);
372378
}
373-
} else {
374-
validateChildKeys(children, type);
375379
}
376380
}
377-
}
378381

379-
if (__DEV__) {
380382
if (warnAboutSpreadingKeyToJSX) {
381383
if (hasOwnProperty.call(props, 'key')) {
382-
console.error(
383-
'React.jsx: Spreading a key to JSX is a deprecated pattern. ' +
384-
'Explicitly pass a key after spreading props in your JSX call. ' +
385-
'E.g. <%s {...props} key={key} />',
386-
getComponentNameFromType(type) || 'ComponentName',
387-
);
384+
const componentName = getComponentNameFromType(type);
385+
const keys = Object.keys(props).filter(k => k !== 'key');
386+
const beforeExample =
387+
keys.length > 0
388+
? '{key: someKey, ' + keys.join(': ..., ') + ': ...}'
389+
: '{key: someKey}';
390+
if (!didWarnAboutKeySpread[componentName + beforeExample]) {
391+
const afterExample =
392+
keys.length > 0 ? '{' + keys.join(': ..., ') + ': ...}' : '{}';
393+
console.error(
394+
'A props object containing a "key" prop is being spread into JSX:\n' +
395+
' let props = %s;\n' +
396+
' <%s {...props} />\n' +
397+
'React keys must be passed directly to JSX without using spread:\n' +
398+
' let props = %s;\n' +
399+
' <%s key={someKey} {...props} />',
400+
beforeExample,
401+
componentName,
402+
afterExample,
403+
componentName,
404+
);
405+
didWarnAboutKeySpread[componentName + beforeExample] = true;
406+
}
388407
}
389408
}
390-
}
391409

392-
if (type === REACT_FRAGMENT_TYPE) {
393-
validateFragmentProps(element);
394-
} else {
395-
validatePropTypes(element);
396-
}
410+
if (type === REACT_FRAGMENT_TYPE) {
411+
validateFragmentProps(element);
412+
} else {
413+
validatePropTypes(element);
414+
}
397415

398-
return element;
416+
return element;
417+
}
399418
}
400419

401420
// These two functions exist to still get child warnings in dev

‎packages/react/src/__tests__/ReactElementJSX-test.js‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -275,16 +275,19 @@ describe('ReactElement.jsx', () => {
275275
class Parent extends React.Component {
276276
render() {
277277
return JSXRuntime.jsx('div', {
278-
children: [JSXRuntime.jsx(Child, {key: '0'})],
278+
children: [JSXRuntime.jsx(Child, {key: '0', prop: 'hi'})],
279279
});
280280
}
281281
}
282282
expect(() =>
283283
ReactDOM.render(JSXRuntime.jsx(Parent, {}), container),
284284
).toErrorDev(
285-
'Warning: React.jsx: Spreading a key to JSX is a deprecated pattern. ' +
286-
'Explicitly pass a key after spreading props in your JSX call. ' +
287-
'E.g. <Child {...props} key={key} />',
285+
'Warning: A props object containing a "key" prop is being spread into JSX:\n' +
286+
' let props = {key: someKey, prop: ...};\n' +
287+
' <Child {...props} />\n' +
288+
'React keys must be passed directly to JSX without using spread:\n' +
289+
' let props = {prop: ...};\n' +
290+
' <Child key={someKey} {...props} />',
288291
);
289292
});
290293
}

‎packages/react/src/jsx/ReactJSXElementValidator.js‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,8 @@ function validateFragmentProps(fragment) {
294294
}
295295
}
296296

297+
const didWarnAboutKeySpread = {};
298+
297299
export function jsxWithValidation(
298300
type,
299301
props,
@@ -390,12 +392,29 @@ export function jsxWithValidation(
390392

391393
if (warnAboutSpreadingKeyToJSX) {
392394
if (hasOwnProperty.call(props, 'key')) {
393-
console.error(
394-
'React.jsx: Spreading a key to JSX is a deprecated pattern. ' +
395-
'Explicitly pass a key after spreading props in your JSX call. ' +
396-
'E.g. <%s {...props} key={key} />',
397-
getComponentNameFromType(type) || 'ComponentName',
398-
);
395+
const componentName = getComponentNameFromType(type);
396+
const keys = Object.keys(props).filter(k => k !== 'key');
397+
const beforeExample =
398+
keys.length > 0
399+
? '{key: someKey, ' + keys.join(': ..., ') + ': ...}'
400+
: '{key: someKey}';
401+
if (!didWarnAboutKeySpread[componentName + beforeExample]) {
402+
const afterExample =
403+
keys.length > 0 ? '{' + keys.join(': ..., ') + ': ...}' : '{}';
404+
console.error(
405+
'A props object containing a "key" prop is being spread into JSX:\n' +
406+
' let props = %s;\n' +
407+
' <%s {...props} />\n' +
408+
'React keys must be passed directly to JSX without using spread:\n' +
409+
' let props = %s;\n' +
410+
' <%s key={someKey} {...props} />',
411+
beforeExample,
412+
componentName,
413+
afterExample,
414+
componentName,
415+
);
416+
didWarnAboutKeySpread[componentName + beforeExample] = true;
417+
}
399418
}
400419
}
401420

‎packages/shared/ReactFeatureFlags.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false; // deprecate lat
217217

218218
// Enables a warning when trying to spread a 'key' to an element;
219219
// a deprecated pattern we want to get rid of in the future
220-
export const warnAboutSpreadingKeyToJSX = false;
220+
export const warnAboutSpreadingKeyToJSX = true;
221221

222222
export const warnAboutStringRefs = false;
223223

‎packages/shared/forks/ReactFeatureFlags.native-fb.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
4848
export const enableTrustedTypesIntegration = false;
4949
export const disableTextareaChildren = false;
5050
export const disableModulePatternComponents = false;
51-
export const warnAboutSpreadingKeyToJSX = false;
51+
export const warnAboutSpreadingKeyToJSX = true;
5252
export const enableSuspenseAvoidThisFallback = false;
5353
export const enableSuspenseAvoidThisFallbackFizz = false;
5454
export const enableCPUSuspense = true;

‎packages/shared/forks/ReactFeatureFlags.native-oss.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3838
export const enableTrustedTypesIntegration = false;
3939
export const disableTextareaChildren = false;
4040
export const disableModulePatternComponents = false;
41-
export const warnAboutSpreadingKeyToJSX = false;
41+
export const warnAboutSpreadingKeyToJSX = true;
4242
export const enableSuspenseAvoidThisFallback = false;
4343
export const enableSuspenseAvoidThisFallbackFizz = false;
4444
export const enableCPUSuspense = false;

‎packages/shared/forks/ReactFeatureFlags.test-renderer.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3838
export const enableTrustedTypesIntegration = false;
3939
export const disableTextareaChildren = false;
4040
export const disableModulePatternComponents = false;
41-
export const warnAboutSpreadingKeyToJSX = false;
41+
export const warnAboutSpreadingKeyToJSX = true;
4242
export const enableSuspenseAvoidThisFallback = false;
4343
export const enableSuspenseAvoidThisFallbackFizz = false;
4444
export const enableCPUSuspense = false;

‎packages/shared/forks/ReactFeatureFlags.test-renderer.native.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3838
export const enableTrustedTypesIntegration = false;
3939
export const disableTextareaChildren = false;
4040
export const disableModulePatternComponents = false;
41-
export const warnAboutSpreadingKeyToJSX = false;
41+
export const warnAboutSpreadingKeyToJSX = true;
4242
export const enableComponentStackLocations = false;
4343
export const enableLegacyFBSupport = false;
4444
export const enableFilterEmptyStringAttributesDOM = false;

‎packages/shared/forks/ReactFeatureFlags.test-renderer.www.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3838
export const enableTrustedTypesIntegration = false;
3939
export const disableTextareaChildren = false;
4040
export const disableModulePatternComponents = true;
41-
export const warnAboutSpreadingKeyToJSX = false;
41+
export const warnAboutSpreadingKeyToJSX = true;
4242
export const enableSuspenseAvoidThisFallback = true;
4343
export const enableSuspenseAvoidThisFallbackFizz = false;
4444
export const enableCPUSuspense = false;

‎packages/shared/forks/ReactFeatureFlags.testing.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3838
export const enableTrustedTypesIntegration = false;
3939
export const disableTextareaChildren = false;
4040
export const disableModulePatternComponents = false;
41-
export const warnAboutSpreadingKeyToJSX = false;
41+
export const warnAboutSpreadingKeyToJSX = true;
4242
export const enableSuspenseAvoidThisFallback = false;
4343
export const enableSuspenseAvoidThisFallbackFizz = false;
4444
export const enableCPUSuspense = false;

0 commit comments

Comments
 (0)