Extract errors from Error() and new Error() instead of invariant()

[sebmarkbage]

The lint rule was updated but not the extract-errors script.

To test this, I first removed everything from codes.json, then ran extract-errors and then ran lint. Lint then found a couple of things missing. Namely in the partial renderer and react-suspense-test-utils. Perhaps because these are not included in the build.

However, then I restored codes.json and ran extract-errors again. This time it found a bunch that are not covered by the lint rule. What do we want to do about these?

Some these weren't using invariant before and so wasn't covered. However, they should've been covered by the lint rule, no?

[acdlite]

So I actually intentionally excluded these, mainly because we weren't previously using invariant to minify them and I wanted to keep the scope of the codemod down until we had a chance to discuss what our philosophy is for when to minify versus not.

react/.eslintrc.js

Lines 140 to 160 in 6485ef7

	// These are files where it's OK to have unminified error messages. These
	// are environments where bundle size isn't a concern, like tests
	// or Node.
	files: [
	'packages/react-dom/src/test-utils/**/*.js',
	'packages/react-devtools-shared/**/*.js',
	'packages/react-noop-renderer/**/*.js',
	'packages/react-pg/**/*.js',
	'packages/react-fs/**/*.js',
	'packages/react-refresh/**/*.js',
	'packages/react-server-dom-webpack/**/*.js',
	'packages/react-test-renderer/**/*.js',
	'packages/react-debug-tools/**/*.js',
	'packages/react-devtools-extensions/**/*.js',
	'packages/react-devtools-scheduling-profiler/**/*.js',
	'packages/react-native-renderer/**/*.js',
	'packages/eslint-plugin-react-hooks/**/*.js',
	'packages/jest-react/**/*.js',
	'packages/**/__tests__/*.js',
	'packages/**/npm/*.js',
	],

For example, I noticed that in some packages that only include one or two errors, the formatErrorMessage runtime is more bytes than if you didn't minify at all.

Also an open question whether it's worth minifying packages that aren't downloaded over the network.

But regardless of what we choose I think extract-errors shouldn't extract from packages that we don't also lint against, so maybe they should use the same array of file patterns.

[acdlite]

The other thing I did was add an explicit option to our bundle config to enable/disable minification per package. This was something else I wanted to discuss, because I had noticed the React Native errors weren't being minified, but I wasn't sure of the historical reasons for or against.

react/scripts/rollup/bundles.js

Line 844 in 6485ef7

minifyWithProdErrorCodes: false,

In the meantime, I liked the explicitness of being able to control exactly which prod bundles get minified. Though if we decide we're going to minify all of them then I can remove it.

[sebmarkbage]

It's maybe a bit weird to have a few things be minified and not others in the same bundle.

Maybe it can be a config on the bundle whether it should run the extraction on it. That way you decide when configuring the bundle whether you expect it to be large and whether it'll be on the server or CLI.

The same source file could run on either server or client.

[acdlite]

Jinx!

[acdlite]

Stamping the changes to the extract-error script though maybe we should figure out which files to extract/not-extract before merging and line it up with the lint rule

[sebmarkbage]

Ignoring paths is not enough atm because of:

// eslint-disable-next-line react-internal/prod-error-codes

[sebmarkbage]

I pushed commits showing what happens if we ignore bundles minifyWithProdErrorCodes === false.

[acdlite]

Most of those are legit opt outs. I was thinking we could have the extract-errors script detect those comments and skip them.

As a final safeguard, what the invariant script could do if it found message without a code in a prod build (that slipped through because of a lint config mistake, perhaps), it would output a FIXME and the post-build lint would catch it. We should probably do the same (code is still there in the invariant branch of the transform-errors plug-in, which I haven’t deleted yet for this reason).

[acdlite]

I wrote an alternate implementation that scrapes the messages from the build artifacts: #22516

The Babel plugin (the one that replaces the error codes with formatProdErrorMessage) will output a FIXME comment with the expected message format. Then the extract-errors script will scrape those from the build directory and update codes.json.

I like this because you can run extract-errors on partial build output, without running the whole thing. And because runs on the actual build output instead of the source files.

[facebook-github-bot]

Hi @sebmarkbage!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!