Follow-up improvements to error code extraction infra (#22516)

* Output FIXME during build for unminified errors

The invariant Babel transform used to output a FIXME comment if it
could not find a matching error code. This could happen if there were
a configuration mistake that caused an unminified message to
slip through.

Linting the compiled bundles is the most reliable way to do it because
there's not a one-to-one mapping between source modules and bundles. For
example, the same source module may appear in multiple bundles, some
which are minified and others which aren't.

This updates the transform to output the same messages for Error calls.

The source lint rule is still useful for catching mistakes during
development, to prompt you to update the error codes map before pushing
the PR to CI.

* Don't run error transform in development

We used to run the error transform in both production and development,
because in development it was used to convert `invariant` calls into
throw statements.

Now that don't use `invariant` anymore, we only have to run the
transform for production builds.

* Add ! to FIXME comment so Closure doesn't strip it

Don't love this solution because Closure could change this heuristic,
or we could switch to a differnt compiler that doesn't support it. But
it works.

Could add a bundle that contains an unminified error solely for the
purpose of testing it, but that seems like overkill.

* Alternate extract-errors that scrapes artifacts

The build script outputs a special FIXME comment when it fails to minify
an error message. CI will detect these comments and fail the workflow.

The comments also include the expected error message. So I added an
alternate extract-errors that scrapes unminified messages from the
build artifacts and updates `codes.json`.

This is nice because it works on partial builds. And you can also run it
after the fact, instead of needing build all over again.

* Disable error minification in more bundles

Not worth it because the number of errors does not outweight the size
of the formatProdErrorMessage runtime.

* Run extract-errors script in CI

The lint_build job already checks for unminified errors, but the output
isn't super helpful.

Instead I've added a new job that runs the extract-errors script and
fails the build if `codes.json` changes. It also outputs the expected
diff so you can easily see which messages were missing from the map.

* Replace old extract-errors script with new one

Deletes the old extract-errors in favor of extract-errors2

Author: acdlite

.circleci/config.yml

@@ -209,7 +209,20 @@ jobs:
       - run: yarn workspaces info | head -n -1 > workspace_info.txt
       - *restore_node_modules
       - run: yarn lint-build
-      - run: scripts/circleci/check_minified_errors.sh
+
+  check_error_codes:
+    docker: *docker
+    environment: *environment
+    steps:
+      - checkout
+      - attach_workspace: *attach_workspace
+      - run: yarn workspaces info | head -n -1 > workspace_info.txt
+      - *restore_node_modules
+      - run:
+          name: Search build artifacts for unminified errors
+          command: |
+            yarn extract-errors
+            git diff || (echo "Found unminified errors. Either update the error codes map or disable error minification for the affected build, if appropriate." && false)
 
   yarn_test:
     docker: *docker
@@ -414,6 +427,9 @@ workflows:
       - yarn_lint_build:
           requires:
             - yarn_build_combined
+      - check_error_codes:
+          requires:
+            - yarn_build_combined
       - RELEASE_CHANNEL_stable_yarn_test_dom_fixtures:
           requires:
             - yarn_build_combined

package.json

@@ -114,7 +114,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",
+    "extract-errors": "node scripts/error-codes/extract-errors.js",
     "postinstall": "node node_modules/fbjs-scripts/node/check-dev-engines.js package.json && node ./scripts/flow/createFlowConfigs.js && node ./scripts/yarn/downloadReactIsForPrettyFormat.js",
     "debug-test": "yarn test --deprecated 'yarn test --debug'",
     "test": "node ./scripts/jest/jest-cli.js",

packages/shared/__tests__/ReactError-test.internal.js

@@ -37,6 +37,7 @@ describe('ReactError', () => {
   });
 
   // @gate build === "production"
+  // @gate !source
   it('should error with minified error code', () => {
     expect(() => ReactDOM.render('Hi', null)).toThrowError(
       'Minified React error #200; visit ' +

scripts/error-codes/README.md

@@ -9,7 +9,10 @@ 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/main/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 extract-errors`.
+  can test it by running `yarn extract-errors`. It works by crawling the build
+  artifacts directory, so you need to have either run the build script or
+  downloaded pre-built artifacts (e.g. with `yarn download build`). It works
+  with partial builds, too.
 - [`transform-error-messages`](https://github.com/facebook/react/blob/main/scripts/error-codes/transform-error-messages.js)
   is a Babel pass that rewrites error messages to IDs for a production
   (minified) build.

scripts/error-codes/__tests__/__snapshots__/transform-error-messages.js.snap

@@ -2,94 +2,47 @@
 
 exports[`error transform handles escaped backticks in template string 1`] = `
 "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-Error(__DEV__ ? \\"Expected \`\\" + listener + \\"\` listener to be a function, instead got a value of \`\\" + type + \\"\` type.\\" : _formatProdErrorMessage(231, listener, type));"
+Error(_formatProdErrorMessage(231, listener, type));"
 `;
 
-exports[`error transform should correctly transform invariants that are not in the error codes map 1`] = `
-"import invariant from 'shared/invariant';
-
-/*FIXME (minify-errors-in-prod): Unminified error message in production build!*/
-if (!condition) {
-  throw Error(\\"This is not a real error message.\\");
-}"
+exports[`error transform should not touch other calls or new expressions 1`] = `
+"new NotAnError();
+NotAnError();"
 `;
 
-exports[`error transform should handle escaped characters 1`] = `
-"import invariant from 'shared/invariant';
+exports[`error transform should output FIXME for errors that don't have a matching error code 1`] = `
+"/*! FIXME (minify-errors-in-prod): Unminified error message in production build!*/
 
-/*FIXME (minify-errors-in-prod): Unminified error message in production build!*/
-if (!condition) {
-  throw Error(\\"What's up?\\");
-}"
+/*! <expected-error-format>\\"This is not a real error message.\\"</expected-error-format>*/
+Error('This is not a real error message.');"
 `;
 
-exports[`error transform should not touch other calls or new expressions 1`] = `
-"new NotAnError();
-NotAnError();"
+exports[`error transform should output FIXME for errors that don't have a matching error code, unless opted out with a comment 1`] = `
+"// eslint-disable-next-line react-internal/prod-error-codes
+Error('This is not a real error message.');"
 `;
 
 exports[`error transform should replace error constructors (no new) 1`] = `
 "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-Error(__DEV__ ? 'Do not override existing functions.' : _formatProdErrorMessage(16));"
+Error(_formatProdErrorMessage(16));"
 `;
 
 exports[`error transform should replace error constructors 1`] = `
 "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-Error(__DEV__ ? 'Do not override existing functions.' : _formatProdErrorMessage(16));"
-`;
-
-exports[`error transform should replace simple invariant calls 1`] = `
-"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-import invariant from 'shared/invariant';
-
-if (!condition) {
-  {
-    throw Error(__DEV__ ? \\"Do not override existing functions.\\" : _formatProdErrorMessage(16));
-  }
-}"
+Error(_formatProdErrorMessage(16));"
 `;
 
 exports[`error transform should support error constructors with concatenated messages 1`] = `
 "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-Error(__DEV__ ? \\"Expected \\" + foo + \\" target to \\" + (\\"be an array; got \\" + bar) : _formatProdErrorMessage(7, foo, bar));"
+Error(_formatProdErrorMessage(7, foo, bar));"
 `;
 
 exports[`error transform should support interpolating arguments with concatenation 1`] = `
 "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-Error(__DEV__ ? 'Expected ' + foo + ' target to be an array; got ' + bar : _formatProdErrorMessage(7, foo, bar));"
+Error(_formatProdErrorMessage(7, foo, bar));"
 `;
 
 exports[`error transform should support interpolating arguments with template strings 1`] = `
 "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-Error(__DEV__ ? \\"Expected \\" + foo + \\" target to be an array; got \\" + bar : _formatProdErrorMessage(7, foo, bar));"
-`;
-
-exports[`error transform should support invariant calls with a concatenated template string and args 1`] = `
-"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-import invariant from 'shared/invariant';
-
-if (!condition) {
-  {
-    throw Error(__DEV__ ? \\"Expected a component class, got \\" + Foo + \\".\\" + Bar : _formatProdErrorMessage(18, Foo, Bar));
-  }
-}"
-`;
-
-exports[`error transform should support invariant calls with args 1`] = `
-"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
-import invariant from 'shared/invariant';
-
-if (!condition) {
-  {
-    throw Error(__DEV__ ? \\"Expected \\" + foo + \\" target to be an array; got \\" + bar : _formatProdErrorMessage(7, foo, bar));
-  }
-}"
-`;
-
-exports[`error transform should support noMinify option 1`] = `
-"import invariant from 'shared/invariant';
-
-if (!condition) {
-  throw Error(\\"Do not override existing functions.\\");
-}"
+Error(_formatProdErrorMessage(7, foo, bar));"
 `;

scripts/error-codes/__tests__/transform-error-messages.js

@@ -28,87 +28,46 @@ describe('error transform', () => {
     process.env.NODE_ENV = oldEnv;
   });
 
-  it('should replace simple invariant calls', () => {
-    expect(
-      transform(`
-import invariant from 'shared/invariant';
-invariant(condition, 'Do not override existing functions.');
-`)
-    ).toMatchSnapshot();
-  });
-
-  it('should throw if invariant is not in an expression statement', () => {
-    expect(() => {
-      transform(`
-import invariant from 'shared/invariant';
-cond && invariant(condition, 'Do not override existing functions.');
-`);
-    }).toThrow('invariant() cannot be called from expression context');
-  });
-
-  it('should support invariant calls with args', () => {
-    expect(
-      transform(`
-import invariant from 'shared/invariant';
-invariant(condition, 'Expected %s target to be an array; got %s', foo, bar);
-`)
-    ).toMatchSnapshot();
-  });
-
-  it('should support invariant calls with a concatenated template string and args', () => {
-    expect(
-      transform(`
-import invariant from 'shared/invariant';
-invariant(condition, 'Expected a component class, ' + 'got %s.' + '%s', Foo, Bar);
-`)
-    ).toMatchSnapshot();
-  });
-
-  it('should correctly transform invariants that are not in the error codes map', () => {
+  it('should replace error constructors', () => {
     expect(
       transform(`
-import invariant from 'shared/invariant';
-invariant(condition, 'This is not a real error message.');
+new Error('Do not override existing functions.');
 `)
     ).toMatchSnapshot();
   });
 
-  it('should handle escaped characters', () => {
+  it('should replace error constructors (no new)', () => {
     expect(
       transform(`
-import invariant from 'shared/invariant';
-invariant(condition, 'What\\'s up?');
+Error('Do not override existing functions.');
 `)
     ).toMatchSnapshot();
   });
 
-  it('should support noMinify option', () => {
-    expect(
-      transform(
-        `
-import invariant from 'shared/invariant';
-invariant(condition, 'Do not override existing functions.');
-`,
-        {noMinify: true}
-      )
-    ).toMatchSnapshot();
-  });
-
-  it('should replace error constructors', () => {
+  it("should output FIXME for errors that don't have a matching error code", () => {
     expect(
       transform(`
-new Error('Do not override existing functions.');
+Error('This is not a real error message.');
 `)
     ).toMatchSnapshot();
   });
 
-  it('should replace error constructors (no new)', () => {
-    expect(
-      transform(`
-Error('Do not override existing functions.');
+  it(
+    "should output FIXME for errors that don't have a matching error " +
+      'code, unless opted out with a comment',
+    () => {
+      // TODO: Since this only detects one of many ways to disable a lint
+      // rule, we should instead search for a custom directive (like
+      // no-minify-errors) instead of ESLint. Will need to update our lint
+      // rule to recognize the same directive.
+      expect(
+        transform(`
+// eslint-disable-next-line react-internal/prod-error-codes
+Error('This is not a real error message.');
 `)
-    ).toMatchSnapshot();
-  });
+      ).toMatchSnapshot();
+    }
+  );
 
   it('should not touch other calls or new expressions', () => {
     expect(