Skip to content

use ECMASCRIPT6 for Closure Compiler #2990

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

Merged
merged 2 commits into from
Jul 29, 2020

Conversation

MaxGraey
Copy link
Contributor

This allow us simplify binaryenjs-post.js and use shorthand properties, let / const and etc

@kripken
Copy link
Member

kripken commented Jul 27, 2020

Is binaryen.js ok with not running in older VMs? Maybe ES6 is pretty widespread at this point though.

@MaxGraey
Copy link
Contributor Author

MaxGraey commented Jul 27, 2020

Only 6 years old IE11 doesn't support this but it already stop supporting by Microsoft and has ~2% market share of desktop browsers. Also I don't think somebody uses binaryen.js in such old browsers. So I think we shouldn't care about it.

@kripken
Copy link
Member

kripken commented Jul 28, 2020

Sounds fine to me. Let's hear what other people think, maybe @dcodeIO @RReverser ?

@dcodeIO
Copy link
Contributor

dcodeIO commented Jul 28, 2020

Since this is mostly about enabling features like let and arrow functions, this sounds good to me given how modern the tech we are working on is anyway.

What's still somewhat unclear to me is when to make the switch to ES modules (perhaps once latest node LTS supports it without caveats?) - but this is separate and this PR doesn't imply that we have to switch. Makes me wonder, however, if we can enforce this in some way, like using --language_out=ECMASCRIPT5? Not sure if CC will complain then.

@RReverser
Copy link
Member

Sounds like a good idea to me.

What's still somewhat unclear to me is when to make the switch to ES modules

Does Closure Compiler even finally support modules output? I remember them force-transpiling those regardless of the language, but, admittedly, it's been a while since I looked into it.

@MaxGraey
Copy link
Contributor Author

MaxGraey commented Jul 29, 2020

Does Closure Compiler even finally support modules output?

I guess it should but I can't figure out which compiler flag/options enable this. It seems ECMASCRIPT_2015 should enable it. But it's not worked in playground

@RReverser
Copy link
Member

It seems ECMASCRIPT_2015 should enable it.

It might be just for the input support. As far as I can tell from the docs, they still always transpile them for output.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds like we all agree this is a good idea then, thanks!

@kripken kripken merged commit 74f0eef into WebAssembly:master Jul 29, 2020
@MaxGraey MaxGraey deleted the improve-closure branch July 29, 2020 17:34
@phated
Copy link
Contributor

phated commented Oct 27, 2020

Hey all, we are trying to upgrade the binaryen version inside Grain and it turns out that Js_of_ocaml can't parse ES6 code. Would it be possible to revert this?

@dcodeIO
Copy link
Contributor

dcodeIO commented Oct 27, 2020

Is it expected that Js_of_ocaml tries to actually parse external JS? My intuition would be that while it generates JS, it should also be possible to otherwise combine the JS it generates with external JS, so we may be able to avoid reverting this PR.

@MaxGraey
Copy link
Contributor Author

MaxGraey commented Oct 27, 2020

Also you could use babel and some pre-processing for lowering (transpiling) ES6 to ES5

@RReverser
Copy link
Member

+1 to previous comments. ES6 has been around for half a decade now, and not going to go away, so no point in reverting and waiting any further, especially when it allows to generate smaller and faster code. Other tools should either ignore the autogenerated JS, or use 3rd-party tools to combine it with any other.

@phated
Copy link
Contributor

phated commented Oct 27, 2020

Please don't dogpile on the messenger. There is many long-standing issues on Js_of_ocaml to fix/switch the (handrolled) parser but they don't seem interested in doing it themselves.

Is it expected that Js_of_ocaml tries to actually parse external JS?

They actually parse the JS files, which is what is crashing currently. They do this because the organizations using Js_of_ocaml are usually OCaml orgs that don't have JS tooling/bundlers/etc. Js_of_ocaml doesn't even support node require, so they do the optimizations/bundling/etc themselves.

I explored ways to avoid this last night and none of them will work for a library that is shipped to opam, like our Binaryen.ml bindings.

Also you could use babel and some pre-processing for lowering (transpiling) ES6 to ES5

We submodule Binaryen and Binaryen.js into our distribution to opam, the binaryen build is fine because the consumers have cmake and gcc on their computers already, but these consumers won't have the tooling to transpile the code.

ES6 has been around for half a decade now, and not going to go away, so no point in reverting and waiting any further, especially when it allows to generate smaller and faster code. Other tools should either ignore the autogenerated JS, or use 3rd-party tools to combine it with any other.

I agree with you, but the OCaml ecosystem has been around for 25 years, and you aren't going to change their workflows. I'm just trying to get Binaryen in the hands of as many people as possible, but you seem to be optimizing for something else entirely.

@MaxGraey
Copy link
Contributor Author

MaxGraey commented Oct 27, 2020

We submodule Binaryen and Binaryen.js into our distribution to opam, the binaryen build is fine because the consumers have cmake and gcc on their computers already, but these consumers won't have the tooling to transpile the code.

If you use own distribution to opam you probably could transpile from ES6 to ES5 only once on your side before publishing. On CI for example or as git hook or somehow in another way

@dcodeIO
Copy link
Contributor

dcodeIO commented Oct 27, 2020

I'm just trying to get Binaryen in the hands of as many people as possible, but you seem to be optimizing for something else entirely.

If there really is no good alternative, we can still go and publish a pre-transpiled @binaryen/es5 package or the like I guess. An even more forward thinking option could be to switch Binaryen's CMake task to even go a step further and emit an ESM module, and then in Binaryen.js we build 1) a bleeding-edge ESM module and 2) a compatibility-focused UMD module including downleveling to ES5. If that makes sense :)

@phated
Copy link
Contributor

phated commented Oct 27, 2020

@MaxGraey how is that any different or better than us just having to maintain yet-another-fork that just tells closure compiler to output es5? It's a 1 line change opposed to even more transpilation and room for error.

@phated
Copy link
Contributor

phated commented Oct 27, 2020

@dcodeIO yeah, I think that could work well!

@RReverser
Copy link
Member

They actually parse the JS files, which is what is crashing currently. They do this because the organizations using Js_of_ocaml are usually OCaml orgs that don't have JS tooling/bundlers/etc. Js_of_ocaml doesn't even support node require, so they do the optimizations/bundling/etc themselves.

If they don't rely on JS tooling, what do they use to parse the JS files? And, more importantly, I'm curious why do they need to parse the generated JS at all?

I don't mean to dogpile, just trying to understand the issue and the best solution here.

@phated
Copy link
Contributor

phated commented Oct 27, 2020

They wrote their own JS parser in OCaml; I assume that was part of their research paper that lead to JSOO. From the research I did (I don't actually work on JSOO), it seems that they apply their own optimizations (like closure compiler?) to the JS files being combined. I tried disabling every optimization, but they still parse the JS files unfortunately.

The 4 year old issue makes a suggestion to look into the flow parser, but switching their hand-rolled one for it seems like a massive undertaking.

@RReverser
Copy link
Member

to the JS files being combined. I tried disabling every optimization, but they still parse the JS files unfortunately

Ah that's interesting. Sounds like a long-term solution would be for them to add a noParse mode similarly to Webpack that just skips parsing entirely for given files and combines them verbatim with others. I understand it's likely not a quick change though.

The 4 year old issue makes a suggestion to look into the flow parser, but switching their hand-rolled one for it seems like a massive undertaking.

Heh, yeah, this was going to be my 2nd suggestion, but it definitely requires even more work. I think skipping parsing would be more sustainable long-term anyway (not to mentioned that it would improve performance, too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants