Skip to content

Commit

Permalink
Turn on key spread warning in jsx-runtime for everyone (#25697)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sebmarkbage authored and acdlite committed Apr 16, 2024
1 parent c2a246e commit 73bfaa1
Show file tree
Hide file tree
Showing 11 changed files with 149 additions and 108 deletions.
199 changes: 109 additions & 90 deletions packages/react/src/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,8 @@ function validateFragmentProps(fragment) {
}
}

const didWarnAboutKeySpread = {};

export function jsxWithValidation(
type,
props,
Expand All @@ -287,115 +289,132 @@ export function jsxWithValidation(
source,
self,
) {
const validType = isValidElementType(type);

// We warn in this case but don't throw. We expect the element creation to
// succeed and there will likely be errors in render.
if (!validType) {
let info = '';
if (
type === undefined ||
(typeof type === 'object' &&
type !== null &&
Object.keys(type).length === 0)
) {
info +=
' You likely forgot to export your component from the file ' +
"it's defined in, or you might have mixed up default and named imports.";
}
if (__DEV__) {
const validType = isValidElementType(type);

// We warn in this case but don't throw. We expect the element creation to
// succeed and there will likely be errors in render.
if (!validType) {
let info = '';
if (
type === undefined ||
(typeof type === 'object' &&
type !== null &&
Object.keys(type).length === 0)
) {
info +=
' You likely forgot to export your component from the file ' +
"it's defined in, or you might have mixed up default and named imports.";
}

const sourceInfo = getSourceInfoErrorAddendum(source);
if (sourceInfo) {
info += sourceInfo;
} else {
info += getDeclarationErrorAddendum();
}
const sourceInfo = getSourceInfoErrorAddendum(source);
if (sourceInfo) {
info += sourceInfo;
} else {
info += getDeclarationErrorAddendum();
}

let typeString;
if (type === null) {
typeString = 'null';
} else if (isArray(type)) {
typeString = 'array';
} else if (type !== undefined && type.$$typeof === REACT_ELEMENT_TYPE) {
typeString = `<${getComponentNameFromType(type.type) || 'Unknown'} />`;
info =
' Did you accidentally export a JSX literal instead of a component?';
} else {
typeString = typeof type;
}
let typeString;
if (type === null) {
typeString = 'null';
} else if (isArray(type)) {
typeString = 'array';
} else if (type !== undefined && type.$$typeof === REACT_ELEMENT_TYPE) {
typeString = `<${getComponentNameFromType(type.type) || 'Unknown'} />`;
info =
' Did you accidentally export a JSX literal instead of a component?';
} else {
typeString = typeof type;
}

if (__DEV__) {
console.error(
'React.jsx: type is invalid -- expected a string (for ' +
'built-in components) or a class/function (for composite ' +
'components) but got: %s.%s',
typeString,
info,
);
if (__DEV__) {
console.error(
'React.jsx: type is invalid -- expected a string (for ' +
'built-in components) or a class/function (for composite ' +
'components) but got: %s.%s',
typeString,
info,
);
}
}
}

const element = jsxDEV(type, props, key, source, self);

// The result can be nullish if a mock or a custom function is used.
// TODO: Drop this when these are no longer allowed as the type argument.
if (element == null) {
return element;
}

// Skip key warning if the type isn't valid since our key validation logic
// doesn't expect a non-string/function type and can throw confusing errors.
// We don't want exception behavior to differ between dev and prod.
// (Rendering will throw with a helpful message and as soon as the type is
// fixed, the key warnings will appear.)
const element = jsxDEV(type, props, key, source, self);

if (validType) {
const children = props.children;
if (children !== undefined) {
if (isStaticChildren) {
if (isArray(children)) {
for (let i = 0; i < children.length; i++) {
validateChildKeys(children[i], type);
}
// The result can be nullish if a mock or a custom function is used.
// TODO: Drop this when these are no longer allowed as the type argument.
if (element == null) {
return element;
}

if (Object.freeze) {
Object.freeze(children);
// Skip key warning if the type isn't valid since our key validation logic
// doesn't expect a non-string/function type and can throw confusing errors.
// We don't want exception behavior to differ between dev and prod.
// (Rendering will throw with a helpful message and as soon as the type is
// fixed, the key warnings will appear.)

if (validType) {
const children = props.children;
if (children !== undefined) {
if (isStaticChildren) {
if (isArray(children)) {
for (let i = 0; i < children.length; i++) {
validateChildKeys(children[i], type);
}

if (Object.freeze) {
Object.freeze(children);
}
} else {
if (__DEV__) {
console.error(
'React.jsx: Static children should always be an array. ' +
'You are likely explicitly calling React.jsxs or React.jsxDEV. ' +
'Use the Babel transform instead.',
);
}
}
} else {
if (__DEV__) {
console.error(
'React.jsx: Static children should always be an array. ' +
'You are likely explicitly calling React.jsxs or React.jsxDEV. ' +
'Use the Babel transform instead.',
);
}
validateChildKeys(children, type);
}
} else {
validateChildKeys(children, type);
}
}
}

if (__DEV__) {
if (warnAboutSpreadingKeyToJSX) {
if (hasOwnProperty.call(props, 'key')) {
console.error(
'React.jsx: Spreading a key to JSX is a deprecated pattern. ' +
'Explicitly pass a key after spreading props in your JSX call. ' +
'E.g. <%s {...props} key={key} />',
getComponentNameFromType(type) || 'ComponentName',
);
const componentName = getComponentNameFromType(type);
const keys = Object.keys(props).filter(k => k !== 'key');
const beforeExample =
keys.length > 0
? '{key: someKey, ' + keys.join(': ..., ') + ': ...}'
: '{key: someKey}';
if (!didWarnAboutKeySpread[componentName + beforeExample]) {
const afterExample =
keys.length > 0 ? '{' + keys.join(': ..., ') + ': ...}' : '{}';
console.error(
'A props object containing a "key" prop is being spread into JSX:\n' +
' let props = %s;\n' +
' <%s {...props} />\n' +
'React keys must be passed directly to JSX without using spread:\n' +
' let props = %s;\n' +
' <%s key={someKey} {...props} />',
beforeExample,
componentName,
afterExample,
componentName,
);
didWarnAboutKeySpread[componentName + beforeExample] = true;
}
}
}
}

if (type === REACT_FRAGMENT_TYPE) {
validateFragmentProps(element);
} else {
validatePropTypes(element);
}
if (type === REACT_FRAGMENT_TYPE) {
validateFragmentProps(element);
} else {
validatePropTypes(element);
}

return element;
return element;
}
}

// These two functions exist to still get child warnings in dev
Expand Down
11 changes: 7 additions & 4 deletions packages/react/src/__tests__/ReactElementJSX-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -429,16 +429,19 @@ describe('ReactElement.jsx', () => {
class Parent extends React.Component {
render() {
return JSXRuntime.jsx('div', {
children: [JSXRuntime.jsx(Child, {key: '0'})],
children: [JSXRuntime.jsx(Child, {key: '0', prop: 'hi'})],
});
}
}
expect(() =>
ReactDOM.render(JSXRuntime.jsx(Parent, {}), container),
).toErrorDev(
'Warning: React.jsx: Spreading a key to JSX is a deprecated pattern. ' +
'Explicitly pass a key after spreading props in your JSX call. ' +
'E.g. <Child {...props} key={key} />',
'Warning: A props object containing a "key" prop is being spread into JSX:\n' +
' let props = {key: someKey, prop: ...};\n' +
' <Child {...props} />\n' +
'React keys must be passed directly to JSX without using spread:\n' +
' let props = {prop: ...};\n' +
' <Child key={someKey} {...props} />',
);
});
}
Expand Down
31 changes: 25 additions & 6 deletions packages/react/src/jsx/ReactJSXElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,8 @@ function validateFragmentProps(fragment) {
}
}

const didWarnAboutKeySpread = {};

export function jsxWithValidation(
type,
props,
Expand Down Expand Up @@ -390,12 +392,29 @@ export function jsxWithValidation(

if (warnAboutSpreadingKeyToJSX) {
if (hasOwnProperty.call(props, 'key')) {
console.error(
'React.jsx: Spreading a key to JSX is a deprecated pattern. ' +
'Explicitly pass a key after spreading props in your JSX call. ' +
'E.g. <%s {...props} key={key} />',
getComponentNameFromType(type) || 'ComponentName',
);
const componentName = getComponentNameFromType(type);
const keys = Object.keys(props).filter(k => k !== 'key');
const beforeExample =
keys.length > 0
? '{key: someKey, ' + keys.join(': ..., ') + ': ...}'
: '{key: someKey}';
if (!didWarnAboutKeySpread[componentName + beforeExample]) {
const afterExample =
keys.length > 0 ? '{' + keys.join(': ..., ') + ': ...}' : '{}';
console.error(
'A props object containing a "key" prop is being spread into JSX:\n' +
' let props = %s;\n' +
' <%s {...props} />\n' +
'React keys must be passed directly to JSX without using spread:\n' +
' let props = %s;\n' +
' <%s key={someKey} {...props} />',
beforeExample,
componentName,
afterExample,
componentName,
);
didWarnAboutKeySpread[componentName + beforeExample] = true;
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false; // deprecate lat

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

export const warnAboutStringRefs = true;

Expand Down
2 changes: 1 addition & 1 deletion packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrustedTypesIntegration = false;
export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnAboutSpreadingKeyToJSX = true;
export const enableSuspenseAvoidThisFallback = false;
export const enableSuspenseAvoidThisFallbackFizz = false;
export const enableCPUSuspense = true;
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrustedTypesIntegration = false;
export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnAboutSpreadingKeyToJSX = true;
export const enableSuspenseAvoidThisFallback = false;
export const enableSuspenseAvoidThisFallbackFizz = false;
export const enableCPUSuspense = false;
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrustedTypesIntegration = false;
export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnAboutSpreadingKeyToJSX = true;
export const enableSuspenseAvoidThisFallback = false;
export const enableSuspenseAvoidThisFallbackFizz = false;
export const enableCPUSuspense = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrustedTypesIntegration = false;
export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnAboutSpreadingKeyToJSX = true;
export const enableComponentStackLocations = false;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrustedTypesIntegration = false;
export const disableTextareaChildren = false;
export const disableModulePatternComponents = true;
export const warnAboutSpreadingKeyToJSX = false;
export const warnAboutSpreadingKeyToJSX = true;
export const enableSuspenseAvoidThisFallback = true;
export const enableSuspenseAvoidThisFallbackFizz = false;
export const enableCPUSuspense = false;
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrustedTypesIntegration = false;
export const disableTextareaChildren = false;
export const disableModulePatternComponents = false;
export const warnAboutSpreadingKeyToJSX = false;
export const warnAboutSpreadingKeyToJSX = true;
export const enableSuspenseAvoidThisFallback = false;
export const enableSuspenseAvoidThisFallbackFizz = false;
export const enableCPUSuspense = false;
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrustedTypesIntegration = false;
export const disableTextareaChildren = __EXPERIMENTAL__;
export const disableModulePatternComponents = true;
export const warnAboutSpreadingKeyToJSX = false;
export const warnAboutSpreadingKeyToJSX = true;
export const enableSuspenseAvoidThisFallback = true;
export const enableSuspenseAvoidThisFallbackFizz = false;
export const enableCPUSuspense = true;
Expand Down

0 comments on commit 73bfaa1

Please sign in to comment.