From cbcdd7a4a3f8c2e6eac29662b2bd62731cf6628d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 28 May 2019 16:16:07 -0700 Subject: [PATCH 1/2] Lint rule for unminified errors Add a lint rule that fails if an invariant message is not part of the error code map. The goal is to be more disciplined about adding and modifiying production error codes. Error codes should be consistent across releases even if their wording changes, for continuity in logs. Currently, error codes are added to the error code map via an automated script that runs right before release. The problem with this approach is that if someone modifies an error message in the source, but neglects to modify the corresponding message in the error code map, then the message will be assigned a new error code, instead of reusing the existing one. Because the error extraction script only runs before a release, people rarely modify the error code map in practice. By moving the extraction step to the PR stage, it forces the author to consider whether the message should be assigned a new error code. It also allows the reviewer to review the changes. The trade off is that it requires more effort and context to land new error messages, or to modify existing ones, particular for new contributors who are not familiar with our processes. Since we already expect users to lint their code, I would argue the additional burden is marginal. Even if they forget to run the lint command locally, they will get quick feedback from the CI lint job, which typically finishes within 2-3 minutes. --- ...arning-and-invariant-args-test.internal.js | 19 ++++++++++++-- .../warning-and-invariant-args.js | 25 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/scripts/eslint-rules/__tests__/warning-and-invariant-args-test.internal.js b/scripts/eslint-rules/__tests__/warning-and-invariant-args-test.internal.js index 7e86014b61822..1ce6e8084db6d 100644 --- a/scripts/eslint-rules/__tests__/warning-and-invariant-args-test.internal.js +++ b/scripts/eslint-rules/__tests__/warning-and-invariant-args-test.internal.js @@ -17,8 +17,10 @@ ruleTester.run('eslint-rules/warning-and-invariant-args', rule, { valid: [ "warning(true, 'hello, world');", "warning(true, 'expected %s, got %s', 42, 24);", - "invariant(true, 'hello, world');", - "invariant(true, 'expected %s, got %s', 42, 24);", + 'arbitraryFunction(a, b)', + // These messages are in the error code map + "invariant(false, 'Do not override existing functions.')", + "invariant(false, '%s(...): Target container is not a DOM element.', str)", ], invalid: [ { @@ -96,5 +98,18 @@ ruleTester.run('eslint-rules/warning-and-invariant-args', rule, { }, ], }, + { + code: "invariant(false, 'Not in error map')", + errors: [ + { + message: + 'Error message does not have a corresponding production error code.\n\n' + + 'Run `yarn build --extract-errors` to add the message to error code map, ' + + 'so it can be stripped from the production builds. Alternatively, if ' + + "you're updating an existing error message, you can modify " + + '`scripts/error-codes/codes.json` directly.', + }, + ], + }, ], }); diff --git a/scripts/eslint-rules/warning-and-invariant-args.js b/scripts/eslint-rules/warning-and-invariant-args.js index 6da11aee0ce5b..f41e5b70d205b 100644 --- a/scripts/eslint-rules/warning-and-invariant-args.js +++ b/scripts/eslint-rules/warning-and-invariant-args.js @@ -9,6 +9,16 @@ 'use strict'; +const fs = require('fs'); +const path = require('path'); +const existingErrorMap = JSON.parse( + fs.readFileSync(path.resolve(__dirname, '../error-codes/codes.json')) +); +const messages = new Set(); +Object.keys(existingErrorMap).forEach(key => + messages.add(existingErrorMap[key]) +); + /** * The warning() and invariant() functions take format strings as their second * argument. @@ -79,6 +89,21 @@ module.exports = function(context) { } ); } + + if (node.callee.name === 'invariant') { + if (!messages.has(format)) { + context.report( + node, + 'Error message does not have a corresponding production ' + + 'error code.\n\n' + + 'Run `yarn build --extract-errors` to add the message to error ' + + 'code map, so it can be stripped from the production builds. ' + + "Alternatively, if you're updating an existing error " + + 'message, you can modify ' + + '`scripts/error-codes/codes.json` directly.' + ); + } + } }, }; }; From db76fba406f475a442d473a273740d21fa4a948a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 28 May 2019 17:18:18 -0700 Subject: [PATCH 2/2] Add unreleased error messages to map --- package.json | 1 + .../InitializeNativeFabricUIManager.js | 2 ++ .../ReactNativeViewConfigRegistry.js | 2 ++ .../Libraries/ReactPrivate/UIManager.js | 2 ++ .../react-reconciler/src/ReactFiberHostConfig.js | 2 ++ .../react-stream/src/ReactFizzFormatConfig.js | 2 ++ packages/react-stream/src/ReactFizzHostConfig.js | 2 ++ scripts/error-codes/README.md | 4 +--- scripts/error-codes/codes.json | 16 ++++++++++++++-- .../warning-and-invariant-args-test.internal.js | 8 ++++---- .../eslint-rules/warning-and-invariant-args.js | 4 ++-- .../ReactNativeViewConfigRegistry.js | 2 ++ 12 files changed, 36 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index 97998b6b6d4f7..2b7fe127ef9a0 100644 --- a/package.json +++ b/package.json @@ -97,6 +97,7 @@ "linc": "node ./scripts/tasks/linc.js", "lint": "node ./scripts/tasks/eslint.js", "lint-build": "node ./scripts/rollup/validate/index.js", + "extract-errors": "yarn build --type=dev --extract-errors", "postinstall": "node node_modules/fbjs-scripts/node/check-dev-engines.js package.json && node ./scripts/flow/createFlowConfigs.js", "debug-test": "cross-env NODE_ENV=development node --inspect-brk node_modules/.bin/jest --config ./scripts/jest/config.source.js --runInBand", "test": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.source.js", diff --git a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/InitializeNativeFabricUIManager.js b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/InitializeNativeFabricUIManager.js index 856c039f470b4..84a00eeaa897b 100644 --- a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/InitializeNativeFabricUIManager.js +++ b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/InitializeNativeFabricUIManager.js @@ -5,6 +5,8 @@ * LICENSE file in the root directory of this source tree. */ +/* eslint-disable react-internal/warning-and-invariant-args */ + 'use strict'; // Mock of the Native Hooks diff --git a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativeViewConfigRegistry.js b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativeViewConfigRegistry.js index 47f4338d1c218..77424ad510513 100644 --- a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativeViewConfigRegistry.js +++ b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativeViewConfigRegistry.js @@ -7,6 +7,8 @@ * @flow strict-local */ +/* eslint-disable react-internal/warning-and-invariant-args */ + 'use strict'; import type { diff --git a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js index 41ed47725cefa..43cd100f48a96 100644 --- a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js +++ b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js @@ -5,6 +5,8 @@ * LICENSE file in the root directory of this source tree. */ +/* eslint-disable react-internal/warning-and-invariant-args */ + 'use strict'; // Mock of the Native Hooks diff --git a/packages/react-reconciler/src/ReactFiberHostConfig.js b/packages/react-reconciler/src/ReactFiberHostConfig.js index ae70ca5cda9f6..6b4d9f24fcdc7 100644 --- a/packages/react-reconciler/src/ReactFiberHostConfig.js +++ b/packages/react-reconciler/src/ReactFiberHostConfig.js @@ -7,6 +7,8 @@ * @flow */ +/* eslint-disable react-internal/warning-and-invariant-args */ + import invariant from 'shared/invariant'; // We expect that our Rollup, Jest, and Flow configurations diff --git a/packages/react-stream/src/ReactFizzFormatConfig.js b/packages/react-stream/src/ReactFizzFormatConfig.js index ae70ca5cda9f6..6b4d9f24fcdc7 100644 --- a/packages/react-stream/src/ReactFizzFormatConfig.js +++ b/packages/react-stream/src/ReactFizzFormatConfig.js @@ -7,6 +7,8 @@ * @flow */ +/* eslint-disable react-internal/warning-and-invariant-args */ + import invariant from 'shared/invariant'; // We expect that our Rollup, Jest, and Flow configurations diff --git a/packages/react-stream/src/ReactFizzHostConfig.js b/packages/react-stream/src/ReactFizzHostConfig.js index ae70ca5cda9f6..6b4d9f24fcdc7 100644 --- a/packages/react-stream/src/ReactFizzHostConfig.js +++ b/packages/react-stream/src/ReactFizzHostConfig.js @@ -7,6 +7,8 @@ * @flow */ +/* eslint-disable react-internal/warning-and-invariant-args */ + import invariant from 'shared/invariant'; // We expect that our Rollup, Jest, and Flow configurations diff --git a/scripts/error-codes/README.md b/scripts/error-codes/README.md index 27d9067cd299a..bad3c0fbfb1ad 100644 --- a/scripts/error-codes/README.md +++ b/scripts/error-codes/README.md @@ -9,9 +9,7 @@ provide a better debugging support in production. Check out the blog post the file will never be changed/removed. - [`extract-errors.js`](https://github.com/facebook/react/blob/master/scripts/error-codes/extract-errors.js) is an node script that traverses our codebase and updates `codes.json`. You - can test it by running `yarn build -- --extract-errors`, but you should only - commit changes to this file when running a release. (The release tool will - perform this step automatically.) + can test it by running `yarn extract-errors`. - [`transform-error-messages`](https://github.com/facebook/react/blob/master/scripts/error-codes/transform-error-messages) is a Babel pass that rewrites error messages to IDs for a production (minified) build. diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index d66cff9426fb5..c2b8d3a976ff3 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -222,7 +222,6 @@ "220": "Container does not support insertBefore operation", "221": "Tried to register two views with the same name %s", "222": "View config not found for name %s", - "223": "Trying to release an event instance into a pool of a different type.", "224": "Can't read from currently-mounting component. This error is likely caused by a bug in React. Please file an issue.", "225": "Unexpected object passed to ReactTestInstance constructor (tag: %s). This is probably a bug in React.", "226": "Unsupported component type %s in test renderer. This is probably a bug in React.", @@ -321,5 +320,18 @@ "319": "A dehydrated suspense boundary must commit before trying to render. This is probably a bug in React.", "320": "Expected ReactFiberErrorDialog.showErrorDialog to be a function.", "321": "Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:\n1. You might have mismatching versions of React and the renderer (such as React DOM)\n2. You might be breaking the Rules of Hooks\n3. You might have more than one copy of React in the same app\nSee https://fb.me/react-invalid-hook-call for tips about how to debug and fix this problem.", - "322": "forwardRef requires a render function but was given %s." + "322": "forwardRef requires a render function but was given %s.", + "323": "React has blocked a javascript: URL as a security precaution.%s", + "324": "An event responder context was used outside of an event cycle. Use context.setTimeout() to use asynchronous responder context outside of event cycle .", + "325": "addRootEventTypes() found a duplicate root event type of \"%s\". This might be because the event type exists in the event responder \"rootEventTypes\" array or because of a previous addRootEventTypes() using this root event type.", + "326": "Expected a valid priority level", + "327": "Should not already be working.", + "328": "Should have a work-in-progress.", + "329": "Unknown root exit status.", + "330": "Should be working on an effect.", + "331": "Cannot flush passive effects while already rendering.", + "332": "Unknown priority level.", + "333": "This should have a parent host component initialized. This error is likely caused by a bug in React. Please file an issue.", + "334": "accumulate(...): Accumulated items must not be null or undefined.", + "335": "ReactDOMServer does not yet support the event API." } diff --git a/scripts/eslint-rules/__tests__/warning-and-invariant-args-test.internal.js b/scripts/eslint-rules/__tests__/warning-and-invariant-args-test.internal.js index 1ce6e8084db6d..3bf12ae0282f9 100644 --- a/scripts/eslint-rules/__tests__/warning-and-invariant-args-test.internal.js +++ b/scripts/eslint-rules/__tests__/warning-and-invariant-args-test.internal.js @@ -104,10 +104,10 @@ ruleTester.run('eslint-rules/warning-and-invariant-args', rule, { { message: 'Error message does not have a corresponding production error code.\n\n' + - 'Run `yarn build --extract-errors` to add the message to error code map, ' + - 'so it can be stripped from the production builds. Alternatively, if ' + - "you're updating an existing error message, you can modify " + - '`scripts/error-codes/codes.json` directly.', + 'Run `yarn extract-errors` to add the message to error code map, ' + + 'so it can be stripped from the production builds. ' + + "Alternatively, if you're updating an existing error message, " + + 'you can modify `scripts/error-codes/codes.json` directly.', }, ], }, diff --git a/scripts/eslint-rules/warning-and-invariant-args.js b/scripts/eslint-rules/warning-and-invariant-args.js index f41e5b70d205b..36fb71ed3b1a8 100644 --- a/scripts/eslint-rules/warning-and-invariant-args.js +++ b/scripts/eslint-rules/warning-and-invariant-args.js @@ -96,8 +96,8 @@ module.exports = function(context) { node, 'Error message does not have a corresponding production ' + 'error code.\n\n' + - 'Run `yarn build --extract-errors` to add the message to error ' + - 'code map, so it can be stripped from the production builds. ' + + 'Run `yarn extract-errors` to add the message to error code ' + + 'map, so it can be stripped from the production builds. ' + "Alternatively, if you're updating an existing error " + 'message, you can modify ' + '`scripts/error-codes/codes.json` directly.' diff --git a/scripts/rollup/shims/react-native/ReactNativeViewConfigRegistry.js b/scripts/rollup/shims/react-native/ReactNativeViewConfigRegistry.js index cb27a9d966861..c842eb4b8576b 100644 --- a/scripts/rollup/shims/react-native/ReactNativeViewConfigRegistry.js +++ b/scripts/rollup/shims/react-native/ReactNativeViewConfigRegistry.js @@ -8,6 +8,8 @@ * @flow strict-local */ +/* eslint-disable react-internal/warning-and-invariant-args */ + 'use strict'; import type {