Run Closure on non-minified prod builds, too #28827
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
facebook-github-bot
added
CLA Signed
React Core Team
|
What could be important:
|
|
The headers are still there, I added them post-Closure |
|
Failing tests should be fixed after rebase on master now that #28593 landed. |
|
Nice! |
acdlite
force-pushed
the
run-closure-on-non-minified-too
branch
3 times, most recently
from
604dea8
to
cd1f695
Compare
|
Why don't we let Closure generate the source maps and just conditionally enable the renaming there? Alternatively, do we even need the renaming? Wouldn't that typically happen anyway as a minification step in product bundling (in various build systems?) Since the build is already pretty complicated and slow it'd be nice to avoid adding more steps… |
1 task
|
FYI sizebot failed because the summary is too large: https://app.circleci.com/pipelines/github/facebook/react/52317/workflows/cfa1d5b9-b4cb-40d1-89db-f06f98f94c85/jobs/838157 Summary for this PR can be seen in https://output.circle-artifacts.com/output/job/17d7076d-4cf5-4552-acc0-63e9a29ef9c4/artifacts/0/sizebot-message.md In #28845, we'll fallback to a link to CI with the full report. |
That is indeed what I want to do, this first PR is meant to demonstrate that Closure + a later mangling step (via Terser in this case, since that's what much of the OSS community uses) is equivalent to Closure with mangling enabled. |
|
Regarding this, though:
That's how it works in main already, but the problem is that the source maps map to a pre-Closure "source", which means it doesn't have as much inlining and dead code elimination applied to it. If we are going to ship source maps (which again I'd prefer not to) then it should map to something like the Meta production builds — identical program structure, just without mangled names. |
gnoff
approved these changes
Apr 17, 2024
In facebook#26446 we started publishing non-minified versions of our production build artifacts, along with source maps, for easier debugging of React when running in production mode. The way it's currently set up is that these builds are generated *before* Closure compiler has run. Which means it's missing many of the optimizations that are in the final build, like dead code elimination. This PR changes the build process to run Closure on the non-minified production builds, too, by moving the sourcemap generation to later in the pipeline. The non-minified builds will still preserve the original symbol names, and we'll use Prettier to add back whitespace. This is the exact same approach we've been using for years to generate production builds for Meta. The idea is that the only difference between the minified and non- minified builds is whitespace and symbol mangling. The semantic structure of the program should be identical. To implement this, I disabled symbol mangling when running Closure compiler. Then, in a later step, the symbols are mangled by Terser. This is when the source maps are generated.
acdlite
force-pushed
the
run-closure-on-non-minified-too
branch
from
cd1f695
to
44ca36d
Compare
|
The size diff is too large to display in a single comment. The CircleCI job contains an artifact called 'sizebot-message.md' with the full message. |
github-actions bot
pushed a commit
that referenced
this pull request
Apr 19, 2024
In #26446 we started publishing non-minified versions of our production build artifacts, along with source maps, for easier debugging of React when running in production mode. The way it's currently set up is that these builds are generated *before* Closure compiler has run. Which means it's missing many of the optimizations that are in the final build, like dead code elimination. This PR changes the build process to run Closure on the non-minified production builds, too, by moving the sourcemap generation to later in the pipeline. The non-minified builds will still preserve the original symbol names, and we'll use Prettier to add back whitespace. This is the exact same approach we've been using for years to generate production builds for Meta. The idea is that the only difference between the minified and non- minified builds is whitespace and symbol mangling. The semantic structure of the program should be identical. To implement this, I disabled symbol mangling when running Closure compiler. Then, in a later step, the symbols are mangled by Terser. This is when the source maps are generated. DiffTrain build for commit 0e0b693.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 19, 2024
In #26446 we started publishing non-minified versions of our production build artifacts, along with source maps, for easier debugging of React when running in production mode. The way it's currently set up is that these builds are generated *before* Closure compiler has run. Which means it's missing many of the optimizations that are in the final build, like dead code elimination. This PR changes the build process to run Closure on the non-minified production builds, too, by moving the sourcemap generation to later in the pipeline. The non-minified builds will still preserve the original symbol names, and we'll use Prettier to add back whitespace. This is the exact same approach we've been using for years to generate production builds for Meta. The idea is that the only difference between the minified and non- minified builds is whitespace and symbol mangling. The semantic structure of the program should be identical. To implement this, I disabled symbol mangling when running Closure compiler. Then, in a later step, the symbols are mangled by Terser. This is when the source maps are generated. DiffTrain build for [0e0b693](0e0b693)
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 19, 2024
This disables symbol renaming in production builds. The original variable and function names are preserved. All other forms of compression applied by Closure (dead code elimination, inlining, etc) are unchanged — the final program is identical to what we were producing before, just in a more readable form. The motivation is to make it easier to debug React issues that only occur in production — the same reason we decided to start shipping sourcemaps in facebook#28827 and facebook#28827. However, because most apps run their own minification step on their npm dependencies, it's not necessary for us to minify the symbols before publishing — it'll be handled the app, if desired. This is the same strategy Meta has used to ship React for years. The React build itself has unminified symbols, but they get minified as part of Meta's regular build pipeline. Even if an app does not minify their npm dependencies, gzip covers most of the cost of symbol renaming anyway. This saves us from having to ship sourcemaps, which means even apps that don't have sourcemaps configured will be able to debug the React build as easily as they would any other npm dependency.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 19, 2024
This disables symbol renaming in production builds. The original variable and function names are preserved. All other forms of compression applied by Closure (dead code elimination, inlining, etc) are unchanged — the final program is identical to what we were producing before, just in a more readable form. The motivation is to make it easier to debug React issues that only occur in production — the same reason we decided to start shipping sourcemaps in facebook#28827 and facebook#28827. However, because most apps run their own minification step on their npm dependencies, it's not necessary for us to minify the symbols before publishing — it'll be handled the app, if desired. This is the same strategy Meta has used to ship React for years. The React build itself has unminified symbols, but they get minified as part of Meta's regular build pipeline. Even if an app does not minify their npm dependencies, gzip covers most of the cost of symbol renaming anyway. This saves us from having to ship sourcemaps, which means even apps that don't have sourcemaps configured will be able to debug the React build as easily as they would any other npm dependency.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 19, 2024
This disables symbol renaming in production builds. The original variable and function names are preserved. All other forms of compression applied by Closure (dead code elimination, inlining, etc) are unchanged — the final program is identical to what we were producing before, just in a more readable form. The motivation is to make it easier to debug React issues that only occur in production — the same reason we decided to start shipping sourcemaps in facebook#28827 and facebook#28827. However, because most apps run their own minification step on their npm dependencies, it's not necessary for us to minify the symbols before publishing — it'll be handled the app, if desired. This is the same strategy Meta has used to ship React for years. The React build itself has unminified symbols, but they get minified as part of Meta's regular build pipeline. Even if an app does not minify their npm dependencies, gzip covers most of the cost of symbol renaming anyway. This saves us from having to ship sourcemaps, which means even apps that don't have sourcemaps configured will be able to debug the React build as easily as they would any other npm dependency.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 19, 2024
This disables symbol renaming in production builds. The original variable and function names are preserved. All other forms of compression applied by Closure (dead code elimination, inlining, etc) are unchanged — the final program is identical to what we were producing before, just in a more readable form. The motivation is to make it easier to debug React issues that only occur in production — the same reason we decided to start shipping sourcemaps in facebook#28827 and facebook#28827. However, because most apps run their own minification step on their npm dependencies, it's not necessary for us to minify the symbols before publishing — it'll be handled the app, if desired. This is the same strategy Meta has used to ship React for years. The React build itself has unminified symbols, but they get minified as part of Meta's regular build pipeline. Even if an app does not minify their npm dependencies, gzip covers most of the cost of symbol renaming anyway. This saves us from having to ship sourcemaps, which means even apps that don't have sourcemaps configured will be able to debug the React build as easily as they would any other npm dependency.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 19, 2024
This disables symbol renaming in production builds. The original variable and function names are preserved. All other forms of compression applied by Closure (dead code elimination, inlining, etc) are unchanged — the final program is identical to what we were producing before, just in a more readable form. The motivation is to make it easier to debug React issues that only occur in production — the same reason we decided to start shipping sourcemaps in facebook#28827 and facebook#28827. However, because most apps run their own minification step on their npm dependencies, it's not necessary for us to minify the symbols before publishing — it'll be handled the app, if desired. This is the same strategy Meta has used to ship React for years. The React build itself has unminified symbols, but they get minified as part of Meta's regular build pipeline. Even if an app does not minify their npm dependencies, gzip covers most of the cost of symbol renaming anyway. This saves us from having to ship sourcemaps, which means even apps that don't have sourcemaps configured will be able to debug the React build as easily as they would any other npm dependency.
acdlite
added a commit
that referenced
this pull request
Apr 20, 2024
This disables symbol renaming in production builds. The original variable and function names are preserved. All other forms of compression applied by Closure (dead code elimination, inlining, etc) are unchanged — the final program is identical to what we were producing before, just in a more readable form. The motivation is to make it easier to debug React issues that only occur in production — the same reason we decided to start shipping sourcemaps in #28827 and #28827. However, because most apps run their own minification step on their npm dependencies, it's not necessary for us to minify the symbols before publishing — it'll be handled the app, if desired. This is the same strategy Meta has used to ship React for years. The React build itself has unminified symbols, but they get minified as part of Meta's regular build pipeline. Even if an app does not minify their npm dependencies, gzip covers most of the cost of symbol renaming anyway. This saves us from having to ship sourcemaps, which means even apps that don't have sourcemaps configured will be able to debug the React build as easily as they would any other npm dependency.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 20, 2024
This disables symbol renaming in production builds. The original variable and function names are preserved. All other forms of compression applied by Closure (dead code elimination, inlining, etc) are unchanged — the final program is identical to what we were producing before, just in a more readable form. The motivation is to make it easier to debug React issues that only occur in production — the same reason we decided to start shipping sourcemaps in #28827 and #28827. However, because most apps run their own minification step on their npm dependencies, it's not necessary for us to minify the symbols before publishing — it'll be handled the app, if desired. This is the same strategy Meta has used to ship React for years. The React build itself has unminified symbols, but they get minified as part of Meta's regular build pipeline. Even if an app does not minify their npm dependencies, gzip covers most of the cost of symbol renaming anyway. This saves us from having to ship sourcemaps, which means even apps that don't have sourcemaps configured will be able to debug the React build as easily as they would any other npm dependency. DiffTrain build for [857ee8c](857ee8c)
This was referenced Apr 24, 2024
bigfootjon
pushed a commit
that referenced
this pull request
Apr 25, 2024
In #26446 we started publishing non-minified versions of our production build artifacts, along with source maps, for easier debugging of React when running in production mode. The way it's currently set up is that these builds are generated *before* Closure compiler has run. Which means it's missing many of the optimizations that are in the final build, like dead code elimination. This PR changes the build process to run Closure on the non-minified production builds, too, by moving the sourcemap generation to later in the pipeline. The non-minified builds will still preserve the original symbol names, and we'll use Prettier to add back whitespace. This is the exact same approach we've been using for years to generate production builds for Meta. The idea is that the only difference between the minified and non- minified builds is whitespace and symbol mangling. The semantic structure of the program should be identical. To implement this, I disabled symbol mangling when running Closure compiler. Then, in a later step, the symbols are mangled by Terser. This is when the source maps are generated. DiffTrain build for commit 0e0b693.
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.
In #26446 we started publishing non-minified versions of our production build artifacts, along with source maps, for easier debugging of React when running in production mode.
The way it's currently set up is that these builds are generated before Closure compiler has run. Which means it's missing many of the optimizations that are in the final build, like dead code elimination.
This PR changes the build process to run Closure on the non-minified production builds, too, by moving the sourcemap generation to later in the pipeline.
The non-minified builds will still preserve the original symbol names, and we'll use Prettier to add back whitespace. This is the exact same approach we've been using for years to generate production builds for Meta.
The idea is that the only difference between the minified and non- minified builds is whitespace and symbol mangling. The semantic structure of the program should be identical.
To implement this, I disabled symbol mangling when running Closure compiler. Then, in a later step, the symbols are mangled by Terser. This is when the source maps are generated.