Lint rule to detect unminified errors #22429
Closed
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...7d40b8a 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
lint-prod-error-codes
branch
from
59c4b87 to
13af118
Compare
acdlite
force-pushed
the
lint-prod-error-codes
branch
from
13af118 to
e1d6cd6
Compare
acdlite
force-pushed
the
lint-prod-error-codes
branch
from
e1d6cd6 to
05726d7
Compare
acdlite
force-pushed
the
lint-prod-error-codes
branch
from
62aae4c to
05726d7
Compare
acdlite
force-pushed
the
lint-prod-error-codes
branch
from
455f427 to
6158ba4
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.
Adds a lint rule that detects when an Error constructor is used without a corresponding production error code. We already have this for `invariant`, but not for regular errors, i.e. `throw new Error(msg)`. There's also nothing that enforces the use of `invariant` besides convention. There are some packages where we don't care to minify errors. These are packages that run in environments where bundle size is not a concern, like react-pg. I added an override in the ESLint config to ignore these.
acdlite
force-pushed
the
lint-prod-error-codes
branch
from
6158ba4 to
7d40b8a
Compare
rickhanlonii
approved these changes
Sep 30, 2021
| @@ -127,6 +127,41 @@ module.exports = { | |||
| }, | |||
|
|
|||
| overrides: [ | |||
| { | |||
| // By default, anything error message that appears the packages directory | |||
There was a problem hiding this comment.
Suggested change
| // By default, anything error message that appears the packages directory | |
| // By default, any error message that appears the packages directory |
| @@ -397,6 +397,7 @@ export function resolveError( | |||
| message: string, | |||
| stack: string, | |||
| ): void { | |||
| // eslint-disable-next-line react-internal/prod-error-codes | |||
There was a problem hiding this comment.
Maybe include a reason why?
| @@ -12,19 +12,22 @@ export type StringDecoder = void; | |||
| export const supportsBinaryStreams = false; | |||
|
|
|||
| export function createStringDecoder(): void { | |||
| // eslint-disable-next-line react-internal/prod-error-codes | |||
There was a problem hiding this comment.
What about defining something like an NotImplementedError or UnminifiedError which is automatically ignored by the lint rule and parser?
It reflects that we're not super consistent about which errors we minify and which ones we don't, because there's no existing lint rule that enforces it either way. The first step should be to decide which packages should have minified error messages. I think probably everything that runs in the browser, but not stuff that runs elsewhere. For RN we don't minify currently, which I suppose makes sense since React itself is never downloaded OTA (I think?). If there are exceptions after that we can consider adding an escape hatch, though I would still prefer an inline comment directive (like |
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.
Based on #22428
Adds a lint rule that detects when an Error constructor is used without a corresponding production error code.
We already have this for
invariant, but not for regular errors, i.e.throw new Error(msg). There's also nothing that enforces the use ofinvariantbesides convention.There are some packages where we don't care to minify errors. These are packages that run in environments where bundle size is not a concern, like react-pg. I added an override in the ESLint config to ignore these.