-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Consolidate build process with GCC #11483
Conversation
f3b06ea
to
59287b6
Compare
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.
Almost all the node bundle sizes for production all seem to have regressed. Is this expected?
const shims = Modules.getShims(bundleType, entry, featureFlags); | ||
const plugins = [ | ||
const isProduction = isProductionBundleType(bundleType); | ||
const isInGlobalScope = bundleType === UMD_DEV || bundleType === UMD_PROD; |
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 not sure this is the best name. Maybe isUMDBundle
?
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.
There may be more bundle types where top-level things are declared in global scope. In theory. :-)
); | ||
return plugins; | ||
}), | ||
].filter(Boolean); |
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 not sure this is super obvious as to what’s happening?
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 only see a ~20 byte difference. If that's what you're referring to, yes, it's expected because we no longer try to compress the license header, and it's consistent for DEV and PROD bundles. |
Added a second commit that doesn't change the build output, but breaks out the bundle header/footer logic into a separate file, and restructures it to be more explicit. This makes |
It is easier to understand if we just explicitly type them out.
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.
LGTM
* Consolidate build process with GCC * Record sizes * Refactor header and footer wrapping It is easier to understand if we just explicitly type them out.
This completely gets rid of Uglify in our build process. GCC provides better DCE for the FB bundles (e.g. the persistence path is now dead code eliminated in www, and mutation path is eliminated in CS). And this unifies our build process with how we already build UMD/CJS.
I forked GCC to add google/closure-compiler#2707. Hopefully this gets merged but if not, it’s a pretty simple change to maintain. Then I recompiled the
google-closure-compiler-js
package and pointed Yarn lockfile to my fork (easier than forking another dependency layer up from there). I verified with Yarn folks it’s a supported (although a bit unobvious) way to point dependency to a fork (and this is pretty much what custom registry support relies on).GCC doesn't preserve whitespace so I added Prettier after it for FB/RN bundles.
I also took an opportunity to clean up how we apply Rollup plugins in general, and set up a single chain instead of switches. This should make bundles more uniform across the build targets.
Some DEV sizes decreased because I started dropping per-file license headers. FB PROD sizes decreased because of better DCE and less whitespace.
RN DEV sizes increased because I did not add Closure to them whereas in the past they did use Uglify (for DEV). I'm not sure why it was desirable and it seemed better to make them un-Closure'd just like FB_DEV builds are. But I can change that if you want to. Mainly I just want to keep DEV bundles with comments, and Closure can't preserve them.
And that's about it! I hope to test and maybe land this on Thursday after shipping 16.1.