-
Notifications
You must be signed in to change notification settings - Fork 70
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
Return to using default closure optimization level #143
Return to using default closure optimization level #143
Conversation
ADVANCED_OPTIMIZATION mode is breaking CommonJS users. As a spot fix, we'll return to using SIMPLE_OPTIMIZATION mode.
@dibenede @lukesandberg Hi there! There was a SIGNIFICANT (~5x) reduction in bundle size in
I believe the above code size reduction will significantly reduce the overall code size of gRPC-Web users. I wonder if there is a way forward with re-applying the optimization flags? Or do you believe if we have any fundamental blockers here? Thanks!! |
I'm actively working on being able to re-enable the optimizations. The main issue was it seemed to cause serious problems for our CommonJS users in particular (#141). For now, the problem seems to be getting more stuff tagged for export and fixing up some of the mechanics of how our project generates CommonJS code from handwritten Closure. So, please bear with us for now and I'll let you know when we're preparing a new release. |
@dibenede Thanks so much for the update! Very glad that this is being worked on actively! Very much looking forward to it! 😃 |
@dibenede Do you have any updates on re-enabling the optimizations for 3.21.2? |
The release is blocked on adding tests to make sure everything is still working. As is, the risk of re-breaking commonjs users is too high. For users that are willing to experiment (and deal with source), the optimizations can be turned on at HEAD. |
@dibenede why not make two builds with different optimization levels and release it? I often see js modules where import looks like First random article on this topic: https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html In order to not introduce breaking changes (new import path) for commonjs users, it should be possible to ship two dists, at And add info to the docs like:
Correct me if I'm wrong. |
The fundamental issue is we lack confidence in the process it took to optimize the code (i.e. manually adding a bunch of @export annotations). So, even if we did a separate release, it could be totally broken for all we know and result in more headaches for everyone. |
Ah, ok. I misunderstood the root cause of the problem. |
Is there another plan to address the size issue? |
ADVANCED_OPTIMIZATION mode is breaking CommonJS users. As a spot fix, we'll return to using SIMPLE_OPTIMIZATION mode.
Verified the change by creating a new npm project that depended on the fixed version and deliberately triggered unknown field behavior (
skipField
) and used uint64s. Couldn't test by enabling CommonJS to run the full binary/*_test.js battery because there are other issues, likely related to our rewriting script and exported test deps.Should fix #141