From aecc48d8772bce610dd1dd50fc0258e29b2fdfa2 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Thu, 4 Jun 2015 10:44:47 -0700 Subject: [PATCH 1/3] Fix wording of warning-and-invariant-args linter The wording for an incorrect number of arguments based on counting %s substitutions. The previous wording was backwards and a bit yoda-like leading to possible ambiguity. --- .../__tests__/warning-and-invariant-args-test.js | 10 ++++++---- eslint-rules/warning-and-invariant-args.js | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/eslint-rules/__tests__/warning-and-invariant-args-test.js b/eslint-rules/__tests__/warning-and-invariant-args-test.js index be460f6cfb..bca79f8b75 100644 --- a/eslint-rules/__tests__/warning-and-invariant-args-test.js +++ b/eslint-rules/__tests__/warning-and-invariant-args-test.js @@ -51,8 +51,9 @@ 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', }, ], }, @@ -60,8 +61,9 @@ eslintTester.addRuleTest('eslint-rules/warning-and-invariant-args', { code: "warning(true, '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', }, ], }, diff --git a/eslint-rules/warning-and-invariant-args.js b/eslint-rules/warning-and-invariant-args.js index 332262ed8b..e120810dda 100644 --- a/eslint-rules/warning-and-invariant-args.js +++ b/eslint-rules/warning-and-invariant-args.js @@ -63,8 +63,8 @@ module.exports = function(context) { 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, From c91fd8ac145c7fd437ff0fc113ba72641a234250 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Thu, 4 Jun 2015 10:52:11 -0700 Subject: [PATCH 2/3] Check warning/invariant calls have good messages The `warning` implementation checked that the message is long enough to be useful. See commit f5038829d for more information. It makes more sense to move this into a lint rule, and also to apply it for both `warning` and `invariant`. We can safely remove stuff from the `warning` implementation as we replace the function internally anyways. https://github.com/facebook/react/pull/4021#issuecomment-108694976 @spicyj --- .../warning-and-invariant-args-test.js | 33 ++++++++++++++++++- eslint-rules/warning-and-invariant-args.js | 17 +++++++--- src/shared/vendor/core/warning.js | 7 ---- 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/eslint-rules/__tests__/warning-and-invariant-args-test.js b/eslint-rules/__tests__/warning-and-invariant-args-test.js index bca79f8b75..b6a25b0e11 100644 --- a/eslint-rules/__tests__/warning-and-invariant-args-test.js +++ b/eslint-rules/__tests__/warning-and-invariant-args-test.js @@ -58,7 +58,7 @@ eslintTester.addRuleTest('eslint-rules/warning-and-invariant-args', { ], }, { - code: "warning(true, 'foobar', 'junk argument');", + code: "warning(true, 'foo is a bar under foobar', 'junk argument');", errors: [ { message: @@ -67,6 +67,37 @@ eslintTester.addRuleTest('eslint-rules/warning-and-invariant-args', { }, ], }, + { + 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)', + }, + ], + }, ], }); diff --git a/eslint-rules/warning-and-invariant-args.js b/eslint-rules/warning-and-invariant-args.js index e120810dda..3611935579 100644 --- a/eslint-rules/warning-and-invariant-args.js +++ b/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,8 +58,17 @@ 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, diff --git a/src/shared/vendor/core/warning.js b/src/shared/vendor/core/warning.js index 90365feb7c..4d525db5ab 100644 --- a/src/shared/vendor/core/warning.js +++ b/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. } From 2fa1b2c810ac0462ee9cde5bc6b0c1af158f55e6 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Thu, 4 Jun 2015 13:42:30 -0700 Subject: [PATCH 3/3] Attempt to simplify/fix warnAndMonitorForKeyUse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > The two callers of this function have different warning configs > internally (static_upstream/core/createWarning.js) so we can't sync it > like this without changing behavior. We should just split this out > into two separate warning calls probably – this code is a little > overabstracted. https://github.com/facebook/react/pull/4021#discussion_r31690020 @spicyj I think completely removing warnAndMonitorForKeyUse is a bit difficult, without duplicating a ton of code. This at least ensures that the format string passed to `warning` is unique. Plus, because the FB internal code in question only matches the beginning of the format string, I think there should be zero internal changes that need to be made to support this refactor. --- .../classic/element/ReactElementValidator.js | 74 +++++++++++-------- 1 file changed, 44 insertions(+), 30 deletions(-) diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index 9bf72c6454..a7067ec045 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/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; } /**