Skip to content
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

Upgrade to google-closure-compiler v20230502 #31803

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

davesnx
Copy link
Contributor

@davesnx davesnx commented Dec 16, 2024

Update gcc to 20230502 since #31587 fails with dynamic imports but in the linked issue they mention that might be a regression from 20230502, so trying to install this.

Copy link

vercel bot commented Dec 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2024 0:45am

@davesnx davesnx changed the title Update to 20230502, since supports dymamic imports Upgrade to google-closure-compiler v20230502 Dec 16, 2024
@react-sizebot
Copy link

react-sizebot commented Dec 16, 2024

Comparing: a9bbe34...a2b73eb

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 511.38 kB 511.38 kB = 91.38 kB 91.38 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 517.53 kB 517.53 kB = 92.34 kB 92.34 kB
facebook-www/ReactDOM-prod.classic.js = 593.09 kB 593.09 kB = 104.45 kB 104.45 kB
facebook-www/ReactDOM-prod.modern.js = 583.35 kB 583.35 kB = 102.90 kB 102.90 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 8049ab4

@eps1lon
Copy link
Collaborator

eps1lon commented Dec 16, 2024

Doesn't help with #31578 as far as I can tell.

@davesnx
Copy link
Contributor Author

davesnx commented Dec 16, 2024

I can't test it with codesandbox (fails to fetch from https://react-builds.vercel.app), neither locally (don't have java installed). I pushed a unit test to ensure it, and I guess I should be able to see it on the CI with yarn test --env=development

@eps1lon
Copy link
Collaborator

eps1lon commented Dec 16, 2024

You can test this locally with yarn build react-dom/client --type=NODE_DEV (we apply GCC to
dev bundles as well) and then check the build artifact for "JSCompiler_OptimizeArgumentsArray_". I won't have time to approve CI runs for every push.

@davesnx
Copy link
Contributor Author

davesnx commented Dec 16, 2024

Time to install some Java@11, then!

@davesnx
Copy link
Contributor Author

davesnx commented Dec 16, 2024

After reading about gcc I found nocollapse from https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#nocollapse and added // @nocollapse into arguments.

This seems to do the job fine but hard to say if it's the right solution. After using @noalias (https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#noalias) it does seem to bail out from the optimization for arguments.

Also, it is worth noticing that my unit test is rather useless. If this goes to be the right solution, I might keep this PR just updating the gcc version and pushing another PR.

@eps1lon
Copy link
Collaborator

eps1lon commented Dec 16, 2024

You can also use #31598 as a basis. That lints against the optimization which makes sure new code doesn't forgot to disable this optimization.

@davesnx davesnx marked this pull request as ready for review December 16, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants