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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
i18n - support global locale data #33523
i18n - support global locale data #33523
Changes from all commits
f00d632
e9b7e2a
bbd492f
29957bd
88d71da
2bcf194
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain that the compression is better if you don't capture
undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a bit of strange one. It was originally added here #23265.
Not 100% sure behind the reasoning for using a constant
u
.Perhaps @ocombe can explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So initially it was because we removed the undefined values in the arrays (because they are not necessary):
[1, , ,3]
is valid, and the files were smallerBut then some people complained that it was causing errors in some webpack plugins, so we were forced to re-add the undefined values
We did some comparisons and since all the files are independent (they are not always bundled, some people lazy load them), it was smaller to just define a constant equal to undefined.
Since then the compilers might have gotten smarter, maybe it isn't necessary anymore, you can try and benchmark that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ocombe for the explanation. Data is king! We should indeed measure it.
As it turns out this PR is generating JS files that will not be further processed, so I am thinking that the generation of those files should go through terser to get them as compact as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for just inlining
undefined
and not trying to be compressor.👍 for going through
terser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to update
BUILD.bazel
to pass these generated files throughterser
but it turned out to be a lot more complicated that expected. I suggest we come back to that optimisation task after this PR has landed.