Update error minification script to support Error call expressions #22428
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
React Core Team
|
Comparing: 7d38e4f...2bcea20 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
acdlite
force-pushed
the
minify-error-messages
branch
from
150afa1 to
599e1a1
Compare
acdlite
force-pushed
the
minify-error-messages
branch
5 times, most recently
from
495df2f to
b41dc1c
Compare
acdlite
commented
Sep 25, 2021
| @@ -287,12 +287,10 @@ | |||
| "288": "It is not supported to run the profiling version of a renderer (for example, `react-dom/profiling`) without also replacing the `schedule/tracing` module with `schedule/tracing-profiling`. Your bundler might have a setting for aliasing both modules. Learn more at https://reactjs.org/link/profiling", | |||
| "289": "Function components cannot have refs.", | |||
| "290": "Element ref was specified as a string (%s) but no owner was set. This could happen for one of the following reasons:\n1. You may be adding a ref to a function component\n2. You may be adding a ref to a component that was not created inside a component's render method\n3. You have multiple copies of React loaded\nSee https://reactjs.org/link/refs-must-have-owner for more information.", | |||
| "291": "Log of yielded values is not empty. Call expect(Scheduler).toHaveYielded(...) first.", | |||
There was a problem hiding this comment.
See https://github.com/facebook/react/pull/22428/files#r716105083. Since this message was never part of a public bundle we can remove it.
acdlite
commented
Sep 25, 2021
| "292": "The matcher `toHaveYielded` expects an instance of React Test Renderer.\n\nTry: expect(Scheduler).toHaveYielded(expectedYields)", | ||
| "293": "Context can only be read while React is rendering, e.g. inside the render method or getDerivedStateFromProps.", | ||
| "294": "ReactDOMServer does not yet support Suspense.", | ||
| "295": "ReactDOMServer does not yet support lazy-loaded components.", | ||
| "296": "Log of yielded values is not empty. Call expect(ReactTestRenderer).unstable_toHaveYielded(...) first.", |
acdlite
force-pushed
the
minify-error-messages
branch
from
b41dc1c to
c09d87d
Compare
|
Ironically the server build of React Fetch increased slightly in size because this message started being minified: but since it's the only error message in that package, the cost of the |
acdlite
force-pushed
the
minify-error-messages
branch
from
c09d87d to
d0b70c9
Compare
acdlite
commented
Sep 25, 2021
| 'Log of yielded values is not empty. ' + | ||
| 'Call expect(ReactTestRenderer).unstable_toHaveYielded(...) first.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Noticed this was being minified in jest-react but that's an internal module so there's no reason to. The extract-errors script must have picked this up at some point. Since it's not part of a public bundle we don't have to minify it.
acdlite
force-pushed
the
minify-error-messages
branch
from
d0b70c9 to
7553658
Compare
acdlite
force-pushed
the
minify-error-messages
branch
from
7553658 to
05726d7
Compare
acdlite
force-pushed
the
minify-error-messages
branch
from
bba6bb8 to
45a60e0
Compare
When this code was written, the error codes map (`codes.json`) was created on-the-fly, so we had to lazily require from inside the visitor. Because `codes.json` is now checked into source, we can import it a single time in module scope.
We use a script to minify our error messages in production. Each message is assigned an error code, defined in `scripts/error-codes/codes.json`. Then our build script replaces the messages with a link to our error decoder page, e.g. https://reactjs.org/docs/error-decoder.html/?invariant=92 This enables us to write helpful error messages without increasing the bundle size. Right now, the script only works for `invariant` calls. It does not work if you throw an Error object. This is an old Facebookism that we don't really need, other than the fact that our error minification script relies on it. So, I've updated the script to minify error constructors, too: Input: Error(`A ${adj} message that contains ${noun}`); Output: Error(formatProdErrorMessage(ERR_CODE, adj, noun)); It only works for constructors that are literally named Error, though we could add support for other names, too. As a next step, I will add a lint rule to enforce that errors written this way must have a corresponding error code.
This error message wasn't being minified because it doesn't use invariant. The reason it didn't use invariant is because this particular error is created without begin thrown — it doesn't need to be thrown because it's located inside the error handling part of the runtime. Now that the error minification script supports Error constructors, we can minify it by assigning it a production error code in `scripts/error-codes/codes.json`. To support the use of Error constructors more generally, I will add a lint rule that enforces each message has a corresponding error code.
acdlite
force-pushed
the
minify-error-messages
branch
from
45a60e0 to
2bcea20
Compare
rickhanlonii
approved these changes
Sep 30, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We use a script to minify our error messages in production. Each message is assigned an error code, defined in
scripts/error-codes/codes.json. Then our build script replaces the messages with a link to our error decoder page, e.g. https://reactjs.org/docs/error-decoder.html/?invariant=92This enables us to write helpful error messages without increasing the bundle size.
Right now, the script only works for
invariantcalls. It does not work if you throw an Error object. This is an old Facebookism that we don't really need, other than the fact that our error minification script relies on it.So, I've updated the script to minify error constructors, too:
Input:
Output:
It only works for constructors that are literally named Error, though we could add support for other names, too.
As a next step, I will add a lint rule to enforce that errors written this way must have a corresponding error code.
Test plan
I added Jest tests for the Babel transform.
There was only a single place in our codebase where we used Error instead of
invariant, inthrowException. I assigned it an error code and confirmed it's now being minified.Before:
After:
Note: we should also strip all uses of
getComponentNameFromFiberfrom the production build, but we'll fix that separately.