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.
facebook · Nov 16, 2022 · 07f46ec · 07f46ec
1 parent d65b88d
commit 07f46ec
Show file tree

Hide file tree

Showing 11 changed files with 149 additions and 108 deletions.
diff --git 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).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

diff --git a/packages/react/src/__tests__/ReactElementJSX-test.js b/packages/react/src/__tests__/ReactElementJSX-test.js
@@ -275,16 +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: 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} />',
       );
     });
   }

diff --git 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).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;
+        }
       }
     }
 

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;