From ecc47e2019c8f805b4aaff90cf19df34039dc536 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 16 Nov 2022 16:00:01 -0500 Subject: [PATCH 1/3] Improve key spread warning This should suggest that the key is put first, not last. --- packages/react/src/ReactElementValidator.js | 199 ++++++++++-------- .../src/__tests__/ReactElementJSX-test.js | 9 +- .../react/src/jsx/ReactJSXElementValidator.js | 31 ++- 3 files changed, 140 insertions(+), 99 deletions(-) diff --git a/packages/react/src/ReactElementValidator.js b/packages/react/src/ReactElementValidator.js index 5e114326cd467..3529f395148a8 100644 --- a/packages/react/src/ReactElementValidator.js +++ b/packages/react/src/ReactElementValidator.js @@ -279,6 +279,8 @@ function validateFragmentProps(fragment) { } } +const didWarnAboutKeySpread = {}; + export function jsxWithValidation( type, props, @@ -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); + const beforeExample = '{' + keys.join(': ..., ') + ': ...}'; + if (!didWarnAboutKeySpread[componentName + beforeExample]) { + const keysWithoutKey = keys.filter(k => k !== 'key'); + const afterExample = + keysWithoutKey.length > 0 + ? '{' + keysWithoutKey.join(': ..., ') + ': ...}' + : '{}'; + console.error( + 'An 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={...} {...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 diff --git a/packages/react/src/__tests__/ReactElementJSX-test.js b/packages/react/src/__tests__/ReactElementJSX-test.js index f53e08850e45a..374326198725d 100644 --- a/packages/react/src/__tests__/ReactElementJSX-test.js +++ b/packages/react/src/__tests__/ReactElementJSX-test.js @@ -282,9 +282,12 @@ describe('ReactElement.jsx', () => { 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. ', + 'Warning: An props object containing a "key" prop is being spread into JSX:\n' + + ' let props = {key: ...};\n' + + ' \n' + + 'React keys must be passed directly to JSX without using spread:\n' + + ' let props = {};\n' + + ' ', ); }); } diff --git a/packages/react/src/jsx/ReactJSXElementValidator.js b/packages/react/src/jsx/ReactJSXElementValidator.js index 6c11219f9fa1a..fb81c872a6337 100644 --- a/packages/react/src/jsx/ReactJSXElementValidator.js +++ b/packages/react/src/jsx/ReactJSXElementValidator.js @@ -294,6 +294,8 @@ function validateFragmentProps(fragment) { } } +const didWarnAboutKeySpread = {}; + export function jsxWithValidation( type, props, @@ -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); + const beforeExample = '{' + keys.join(': ..., ') + ': ...}'; + if (!didWarnAboutKeySpread[componentName + beforeExample]) { + const keysWithoutKey = keys.filter(k => k !== 'key'); + const afterExample = + keysWithoutKey.length > 0 + ? '{' + keysWithoutKey.join(': ..., ') + ': ...}' + : '{}'; + console.error( + 'An 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={...} {...props} />', + beforeExample, + componentName, + afterExample, + componentName, + ); + didWarnAboutKeySpread[componentName + beforeExample] = true; + } } } From 273616146b12ed760254e799f44a14b5f07caff3 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 16 Nov 2022 16:01:17 -0500 Subject: [PATCH 2/3] Turn on key spread warning in jsx-runtime for everyone --- packages/shared/ReactFeatureFlags.js | 2 +- packages/shared/forks/ReactFeatureFlags.native-fb.js | 2 +- packages/shared/forks/ReactFeatureFlags.native-oss.js | 2 +- packages/shared/forks/ReactFeatureFlags.test-renderer.js | 2 +- packages/shared/forks/ReactFeatureFlags.test-renderer.native.js | 2 +- packages/shared/forks/ReactFeatureFlags.test-renderer.www.js | 2 +- packages/shared/forks/ReactFeatureFlags.testing.js | 2 +- packages/shared/forks/ReactFeatureFlags.testing.www.js | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 100124c28b7b4..9008562472e36 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -217,7 +217,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 = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index c4afa33704b20..1777de0519625 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -48,7 +48,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; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index c457dee5ce523..28052ac785658 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -38,7 +38,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; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index dd22b893d239d..88a6513c0b362 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -38,7 +38,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; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index de6d821d3b24a..dcbd65ad715c3 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -38,7 +38,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; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 65e4f8c4da46f..e6e94b66ecf82 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -38,7 +38,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; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 6bf55b61ba397..97c1a0f443aca 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -38,7 +38,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; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index f8fd3ca80587d..e018c20d11f7a 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -38,7 +38,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; From 92daf551dcb0bc0a641644e278fad0d97ef053e9 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 16 Nov 2022 17:17:47 -0500 Subject: [PATCH 3/3] Nits --- packages/react/src/ReactElementValidator.js | 16 ++++++++-------- .../react/src/__tests__/ReactElementJSX-test.js | 10 +++++----- .../react/src/jsx/ReactJSXElementValidator.js | 16 ++++++++-------- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/packages/react/src/ReactElementValidator.js b/packages/react/src/ReactElementValidator.js index 3529f395148a8..b67f52fe9f785 100644 --- a/packages/react/src/ReactElementValidator.js +++ b/packages/react/src/ReactElementValidator.js @@ -382,21 +382,21 @@ export function jsxWithValidation( if (warnAboutSpreadingKeyToJSX) { if (hasOwnProperty.call(props, 'key')) { const componentName = getComponentNameFromType(type); - const keys = Object.keys(props); - const beforeExample = '{' + keys.join(': ..., ') + ': ...}'; + 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 keysWithoutKey = keys.filter(k => k !== 'key'); const afterExample = - keysWithoutKey.length > 0 - ? '{' + keysWithoutKey.join(': ..., ') + ': ...}' - : '{}'; + keys.length > 0 ? '{' + keys.join(': ..., ') + ': ...}' : '{}'; console.error( - 'An props object containing a "key" prop is being spread into JSX:\n' + + '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={...} {...props} />', + ' <%s key={someKey} {...props} />', beforeExample, componentName, afterExample, diff --git a/packages/react/src/__tests__/ReactElementJSX-test.js b/packages/react/src/__tests__/ReactElementJSX-test.js index 374326198725d..854618bf18ee1 100644 --- a/packages/react/src/__tests__/ReactElementJSX-test.js +++ b/packages/react/src/__tests__/ReactElementJSX-test.js @@ -275,19 +275,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: An props object containing a "key" prop is being spread into JSX:\n' + - ' let props = {key: ...};\n' + + 'Warning: A props object containing a "key" prop is being spread into JSX:\n' + + ' let props = {key: someKey, prop: ...};\n' + ' \n' + 'React keys must be passed directly to JSX without using spread:\n' + - ' let props = {};\n' + - ' ', + ' let props = {prop: ...};\n' + + ' ', ); }); } diff --git a/packages/react/src/jsx/ReactJSXElementValidator.js b/packages/react/src/jsx/ReactJSXElementValidator.js index fb81c872a6337..044f9a2b05948 100644 --- a/packages/react/src/jsx/ReactJSXElementValidator.js +++ b/packages/react/src/jsx/ReactJSXElementValidator.js @@ -393,21 +393,21 @@ export function jsxWithValidation( if (warnAboutSpreadingKeyToJSX) { if (hasOwnProperty.call(props, 'key')) { const componentName = getComponentNameFromType(type); - const keys = Object.keys(props); - const beforeExample = '{' + keys.join(': ..., ') + ': ...}'; + 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 keysWithoutKey = keys.filter(k => k !== 'key'); const afterExample = - keysWithoutKey.length > 0 - ? '{' + keysWithoutKey.join(': ..., ') + ': ...}' - : '{}'; + keys.length > 0 ? '{' + keys.join(': ..., ') + ': ...}' : '{}'; console.error( - 'An props object containing a "key" prop is being spread into JSX:\n' + + '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={...} {...props} />', + ' <%s key={someKey} {...props} />', beforeExample, componentName, afterExample,