-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-12393: [JS] [WIP] Drop closure compiler #10183
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
Conversation
| function remove_closure_tmp_files() { | ||
| return del(targetDir(target, `cls`)) | ||
| } | ||
| `build:${esm}`, |
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.
@trxcllnt I don't know much about gulp but this seems to run the same job twice when I run yarn build -t es5. How can I avoid that?
|
Maybe it's worth trying to run closure over the es2015 sources as well (instead of removing closure) if the results are that much better 😆. |
|
@domoritz closure compiler is discontinued, right? I can help looking into alternatives if you need any help with this issue. |
|
Thanks for offering to help. No, I don't think it's discontinued but it's a lot of work to keep it running for potentially small benefits so we are considering to remove it. We already use terser for all non es5 builds. |
|
https://github.com/google/closure-compiler-npm/blob/master/packages/google-closure-compiler-js/readme.md, you are right, it looks like only the JS version is being discontinued |
|
Ah yes, I switched away from that last month when we updated to the latest closure compiler version. |
|
@domoritz it seems like not using Closure Compiler yields a bundle that's 58kb larger. Am I reading that correctly? What's the impact after compression? A larger bundle seems like a regression here. |
|
I haven't had the cycles to actually check. The bundles may not be correct yet so I wouldn't count on these numbers yet. But if they are right, I wonder whether we should use closure for non es5 as well. |
|
Will redo and compare with updated terser as well. |
Before
After