Merge pull request #4029 from PiPeep/warning-invariant-rules-take-two

Fix problems with/improve warning and invariant rules (take two)

eslint-rules/__tests__/warning-and-invariant-args-test.js

@@ -51,17 +51,50 @@ eslintTester.addRuleTest('eslint-rules/warning-and-invariant-args', {
       code: "warning(true, 'expected %s, got %s');",
       errors: [
         {
-          message: 'Expected 4 arguments in call to warning, but got 2 ' +
-                   'based on the number of "%s" substitutions',
+          message:
+            'Expected 4 arguments in call to warning based on the number of ' +
+            '"%s" substitutions, but got 2',
         },
       ],
     },
     {
-      code: "warning(true, 'foobar', 'junk argument');",
+      code: "warning(true, 'foo is a bar under foobar', 'junk argument');",
       errors: [
         {
-          message: 'Expected 2 arguments in call to warning, but got 3 ' +
-                   'based on the number of "%s" substitutions',
+          message:
+            'Expected 2 arguments in call to warning based on the number of ' +
+            '"%s" substitutions, but got 3',
+        },
+      ],
+    },
+    {
+      code: "invariant(true, 'error!');",
+      errors: [
+        {
+          message:
+            'The invariant format should be able to uniquely identify this ' +
+            'invariant. Please, use a more descriptive format than: error!',
+        },
+      ],
+    },
+    {
+      code: "warning(true, 'error!');",
+      errors: [
+        {
+          message:
+            'The warning format should be able to uniquely identify this ' +
+            'warning. Please, use a more descriptive format than: error!',
+        },
+      ],
+    },
+    {
+      code: "warning(true, '%s %s, %s %s: %s (%s)', 1, 2, 3, 4, 5, 6);",
+      errors: [
+        {
+          message:
+            'The warning format should be able to uniquely identify this ' +
+            'warning. Please, use a more descriptive format than: ' +
+            '%s %s, %s %s: %s (%s)',
         },
       ],
     },

eslint-rules/warning-and-invariant-args.js

@@ -49,8 +49,8 @@ module.exports = function(context) {
         );
         return;
       }
-      var str = getLiteralString(node.arguments[1]);
-      if (str === null) {
+      var format = getLiteralString(node.arguments[1]);
+      if (format === null) {
         context.report(
           node,
           'The second argument to {{name}} must be a string literal',
@@ -58,13 +58,22 @@ module.exports = function(context) {
         );
         return;
       }
-      // count the number of string substitutions, plus the first two args
-      var expectedNArgs = (str.match(/%s/g) || []).length + 2;
+      if (format.length < 10 || /^[s\W]*$/.test(format)) {
+        context.report(
+          node,
+          'The {{name}} format should be able to uniquely identify this ' +
+          '{{name}}. Please, use a more descriptive format than: {{format}}',
+          {name: node.callee.name, format: format}
+        );
+        return;
+      }
+      // count the number of formating substitutions, plus the first two args
+      var expectedNArgs = (format.match(/%s/g) || []).length + 2;
       if (node.arguments.length !== expectedNArgs) {
         context.report(
           node,
-          'Expected {{expectedNArgs}} arguments in call to {{name}}, but ' +
-          'got {{length}} based on the number of "%s" substitutions',
+          'Expected {{expectedNArgs}} arguments in call to {{name}} based on ' +
+          'the number of "%s" substitutions, but got {{length}}',
           {
             expectedNArgs: expectedNArgs,
             name: node.callee.name,

src/isomorphic/classic/element/ReactElementValidator.js

@@ -97,10 +97,18 @@ function validateExplicitKey(element, parentType) {
   }
   element._store.validated = true;
 
-  warnAndMonitorForKeyUse(
-    'Each child in an array or iterator should have a unique "key" prop.',
-    element,
-    parentType
+  var addenda = getAddendaForKeyUse('uniqueKey', element, parentType);
+  if (addenda === null) {
+    // we already showed the warning
+    return;
+  }
+  warning(
+    false,
+    'Each child in an array or iterator should have a unique "key" prop.' +
+    '%s%s%s',
+    addenda.parentOrOwner || '',
+    addenda.childOwner || '',
+    addenda.url || ''
   );
 }
 
@@ -117,10 +125,18 @@ function validatePropertyKey(name, element, parentType) {
   if (!NUMERIC_PROPERTY_REGEX.test(name)) {
     return;
   }
-  warnAndMonitorForKeyUse(
-    'Child objects should have non-numeric keys so ordering is preserved.',
-    element,
-    parentType
+  var addenda = getAddendaForKeyUse('numericKeys', element, parentType);
+  if (addenda === null) {
+    // we already showed the warning
+    return;
+  }
+  warning(
+    false,
+    'Child objects should have non-numeric keys so ordering is preserved.' +
+    '%s%s%s',
+    addenda.parentOrOwner || '',
+    addenda.childOwner || '',
+    addenda.url || ''
   );
 }
 
@@ -128,49 +144,47 @@ function validatePropertyKey(name, element, parentType) {
  * Shared warning and monitoring code for the key warnings.
  *
  * @internal
- * @param {string} message The base warning that gets output.
+ * @param {string} messageType A key used for de-duping warnings.
  * @param {ReactElement} element Component that requires a key.
  * @param {*} parentType element's parent's type.
+ * @returns {?object} A set of addenda to use in the warning message, or null
+ * if the warning has already been shown before (and shouldn't be shown again).
  */
-function warnAndMonitorForKeyUse(message, element, parentType) {
+function getAddendaForKeyUse(messageType, element, parentType) {
   var ownerName = getCurrentOwnerDisplayName();
   var parentName = typeof parentType === 'string' ?
     parentType : parentType.displayName || parentType.name;
 
   var useName = ownerName || parentName;
-  var memoizer = ownerHasKeyUseWarning[message] || (
-    ownerHasKeyUseWarning[message] = {}
+  var memoizer = ownerHasKeyUseWarning[messageType] || (
+    ownerHasKeyUseWarning[messageType] = {}
   );
-  if (memoizer.hasOwnProperty(useName)) {
-    return;
+  if (memoizer[useName]) {
+    return null;
   }
   memoizer[useName] = true;
 
-  var parentOrOwnerAddendum =
-    ownerName ? ` Check the render method of ${ownerName}.` :
-    parentName ? ` Check the React.render call using <${parentName}>.` :
-    '';
+  var addenda = {
+    parentOrOwner:
+      ownerName ? ` Check the render method of ${ownerName}.` :
+      parentName ? ` Check the React.render call using <${parentName}>.` :
+      null,
+    url: ' See https://fb.me/react-warning-keys for more information.',
+    childOwner: null,
+  };
 
   // Usually the current owner is the offender, but if it accepts children as a
   // property, it may be the creator of the child that's responsible for
   // assigning it a key.
-  var childOwnerAddendum = '';
   if (element &&
       element._owner &&
       element._owner !== ReactCurrentOwner.current) {
-    // Name of the component that originally created this child.
-    var childOwnerName = getName(element._owner);
-
-    childOwnerAddendum = ` It was passed a child from ${childOwnerName}.`;
+    // Give the component that originally created this child.
+    addenda.childOwner =
+      ` It was passed a child from ${getName(element._owner)}.`;
   }
 
-  warning(
-    false,
-    '%s%s%s See https://fb.me/react-warning-keys for more information.',
-    message,
-    parentOrOwnerAddendum,
-    childOwnerAddendum
-  );
+  return addenda;
 }
 
 /**

src/shared/vendor/core/warning.js

@@ -31,13 +31,6 @@ if (__DEV__) {
       );
     }
 
-    if (format.length < 10 || /^[s\W]*$/.test(format)) {
-      throw new Error(
-        'The warning format should be able to uniquely identify this ' +
-        'warning. Please, use a more descriptive format than: ' + format
-      );
-    }
-
     if (format.indexOf('Failed Composite propType: ') === 0) {
       return; // Ignore CompositeComponent proptype check.
     }