[core] Minify error messages in production #21214
Merged
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
github-actions
bot
added
the
PR: out-of-date
| return response.json(); | ||
| }) | ||
| .then((json) => { | ||
| if (cancelled === false) { |
There was a problem hiding this comment.
I'm curious about what you think of the @dmtrKovalenko's abort approach in https://github.com/mui-org/material-ui-pickers/pull/1829/files#diff-25080e40b618a1b160ccd8949076fea5R29.
There was a problem hiding this comment.
If this would be supported it's definitely a possibility for reducing wasteful fetches. I haven't looked at it because this approach works just as well? Is there some edge case I'm missing?
|
Love this approach 😍 Will it work for errors throwed outside of core? I am wondering because there are couple of errors that would be useful to minify in production. Maybe we can make some kind of namespace in the |
Technically all we need to do is publish The "error-decoder" part is optional i.e. can be solved however you see fit. You could just throw an error code but that doesn't help users much and you'll have to deal with the error reports later. |
eps1lon
force-pushed
the
feat/minify-errors
branch
from
61ea763
to
9e9c1b3
Compare
|
@material-ui/core: parsed: -0.18% 😍, gzip: -0.40% 😍 Details of bundle changes.Comparing: 1596790...f3058db Details of page changes
|
github-actions
bot
removed
the
PR: out-of-date
eps1lon
force-pushed
the
feat/minify-errors
branch
from
1b0240f
to
7968d31
Compare
eps1lon
force-pushed
the
feat/minify-errors
branch
2 times, most recently
from
032a996
to
b925316
Compare
github-actions
bot
added
the
PR: out-of-date
Avoids dealing with custom module resolution or accidentally publishing the file
Otherwise they end up as empty string literals in NODE_ENV=test Don't fully understand why babel needs raw and cooked values
I don't think I fully understand them. Let's not deal with them in strings for code examples.
mixed-barell means it's part re-export part actual export webpack keeps all the re-exports around even if you only import the actual export. Maybe something is off with side-effect detection
less jargon
eps1lon
force-pushed
the
feat/minify-errors
branch
from
b925316
to
75bdd02
Compare
github-actions
bot
removed
the
PR: out-of-date
There was a problem hiding this comment.
Great work, it sounds like we will be able to add more "invariant" like checks :)
- How is versioning of the errors handled?
It seems to be based on an incremental logic. Correct? This sounds smart.
I assume it also solves the ordering issue (no guarantee on which order the Babel will parse the files). - I think that we have a couple of improvement opportunities for the design of the
/production-errorpage. What do you think about these points:- We apply the wrong variant to the body element: body2. The bug is tracked in Applying body2 styling to the body element causes weird behavior that can't be overriden #17100. I think that "The original text of the error you encountered" should apply the
body1style, like the text above it.
- I think that we should leverage the box-shadow as little as possible. What do you think about going closer to:
or 
maybe using the Alert component? https://codesandbox.io/s/material-demo-t3lxn?file=/demo.js
- We apply the wrong variant to the body element: body2. The bug is tracked in Applying body2 styling to the body element causes weird behavior that can't be overriden #17100. I think that "The original text of the error you encountered" should apply the
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
I don't think so. Elevation has a defined semantic in Material design.
This is not an alert.
Should be done in a separate PR. If we apply the wrong typography class, I don't see how this is tied to this PR. |
|
@eps1lon We have started to look for a designer to help us, to revamp the landing page, to improve the layout of the documentation, to build the design of the enterprise product marketing pages, etc. I think that progressively, we will move away from the default look of Material Design, for the design of material-ui.com (not the components). I think that we need to find a design that allows resonating with the different themes we want to provide by default, on top of the default Material Design look. Ideally, I wish we could cover:
Regarding the very case, I would be cautious with this rule "Don’t use shadows for style only".
In the long term, I think that we should defer design decisions to a UI/UX designer, when possible. |
|
Great, but unrelated to this PR. |
|
What do you think of this recommendation?
|
|
It's not for styles only. It outlines the container that holds the error. The container has a meaning that is expressed via styles. |
eps1lon
commented
Jun 3, 2020
github-actions
bot
added
the
PR: out-of-date
Was extracted into module for usage in separate extract-errors scrip. Since we no longer plan on doing this we can inline
eps1lon
removed
the
PR: out-of-date
|
Considering the upvotes as approvals. Other review does not have actionable items. |
|
@eps1lon Awesome! They might be potential by giving a second iteration on the design of the error on the documentation but I think that we should wait to see the traffic on the new page. We probably want to have enough traffic on the page before spending more time on it (assuming we ask a designer to do so, there are pages with more leverage :D). |
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 how react minifies errors.
We currently only wrap console.(error|warn) in
process.env.NODE_ENV !== "production"to strip these oftentimes large or expensive messages for production builds.We don't do this for thrown errors since these affect strack traces and would make debugging very hard at the cost of not having code that is never used for working apps.
Using
babel-plugin-macrosthey end up asthrow new Error(a(1))in production builds. This has a constant bundle size cost sincea(minified fromformatMuiErrorMessage). Logs a message explaining these codes.For authors
If you think an error should be minified you have to throw
new MuiError(from@material-ui/utils/macros/MuiError.macro) instead:The messsage can only be a basic string literal e.g.
'a basic string literal'or multiple string literals concatenated with+e.g.'A basic string literal.\n' 'With a newline concatenated.'. This ensures we can extract the error messages.placeholders i.e. string interpolation
If you want to print variables in your error messages you have to use
printfsyntax similar to node'sconsole.log:throw new MuiError('Expected 1-10 but got %s', inputNumber)inputNumberwill end up in the url to the error decoder page so make sure the value has a useful serialization e.g.element?.tagName.implementation
babel plugin
Input:
Output:
messages in production
To aid debugging in prod we collect a list of error codes that can be decoded on a dedicated page:
/error-decoder?code=[code].Current messages:
choices
babel-plugin-macros
This makes the transformation explicit. Otherwise we need to check our context since we don't always want to transform them (e.g. errors in custom propTypes). It also ensures that the dependency on
@material-ui/utilsis explicit. If this dependency isn't declaredformatMuiErrorMessagewouldn't be available.error extraction during build time
The original plan was to have a separate script for error code extraction but the order in which babel plugins are applied is important and somewhat brittle. By doing transformation and extraction in a single step we won't ever have problems.
This also ensures that we don't have to keep track of the list of transpiled files.
Limiting to string literals and
+concatenationWe need a string that we can
JSON.stringifyinto a.jsonfile. So whatever we encounter inthrow new Error(message)must be evaluated at build time.Array.prototype.joinor template literals make this not trivial. As long as there's no fundamental behavior we're missing with the current syntax limitation I'd rather not spend much time on it. Allowing more syntax would require more code. And the more code the more likely bugs are.more to follow
I probably already forgot some decisions. This is why I opened this before it's ready so that I'm forced to explain while implementing.