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

[CS2] Replace Closure Compiler with Babili, transform browser compiler into ES5-ish #4523

Merged
merged 7 commits into from
Apr 26, 2017

Conversation

GeoffreyBooth
Copy link
Collaborator

Since Closure Compiler can’t minify our browser compiler until google/closure-compiler-js#59 is fixed, I thought we should look into replacing it. Uglify is working on support for ES2015 in its harmony branch, but it seems a bit brittle to rely on a branch in our build. Hence Babili, a.k.a. Babel Compiler. It doesn’t have a Node API, hence the call via execSync, but it seems to work. It outputs a minified coffeescript.js, still using ES2015 syntax.

This raises the question of whether we should go a bit further and run the browser compiler through Babel itself, to transpile the ES2015 down. Babili and potentially Babel would still just be devDependencies, so we wouldn’t be adding Babel to the project proper as a regular dependency for everyone. Making the browser compiler safe for non-evergreen browsers feels like something we should probably do . . .

@GeoffreyBooth GeoffreyBooth added this to the 2.0.0 milestone Apr 22, 2017
@GeoffreyBooth GeoffreyBooth requested a review from lydell April 22, 2017 22:34
@jashkenas
Copy link
Owner

Just a thought, but now that the particular erroneous syntax was identified in the Closure compiler ticket — it might be easier to keep using it, but just change that bit of syntax where it occurs on our end?

@GeoffreyBooth
Copy link
Collaborator Author

Unfortunately, that bit of syntax is destructuring, which we use throughout our codebase.

@lydell
Copy link
Collaborator

lydell commented Apr 24, 2017

It doesn’t have a Node API

The Node API is using it as a Babel preset: https://github.com/babel/babili#babel-preset

I'm not opposed to having Babel as a devDependency. Since we mention running the output of CoffeeScript through some other tool, such as Babel, I think it could be a good basic test as well. If we do use Babel for the browser version, adding Babili on top of that would be easy.

Babili already has Babel as a dependency anyway. I'd say either go all the way with Babel, or stick with Google Closure Compiler.

… to disable transforming if we only want to minify. Unfortunately several browser tests fail when transformed . . .
@GeoffreyBooth
Copy link
Collaborator Author

So I went all the way and added Babel to transform our browser build down to whatever its env preset transpiles to. I also added a check for TRANSFORM=false like we already have MINIFY=false, so you can produce an ES2015+ browser build if you want to.

It all works great, except that 28 browser tests fail unless I use TRANSFORM=false. 26 of them fail with the same error, with a useless stack trace:

ReferenceError: this hasn’t been initialised - super() hasn’t been called

The other two are:

  duplicate formal parameters are prohibited
  AssertionError: Got unwanted exception. ({_,@_})->

  `Future Reserved Word`s, `eval` and `arguments` restrictions
  AssertionError: Got unwanted exception. do eval

So . . . I guess these are bugs in Babel? Since the ES2015+ code passes these tests and works fine in Node 7.9 and Chrome 60. Thoughts?

@GeoffreyBooth GeoffreyBooth changed the title [CS2] Replace Closure Compiler with Babili [WIP][CS2] Replace Closure Compiler with Babili Apr 25, 2017
@lydell
Copy link
Collaborator

lydell commented Apr 25, 2017

The Babel implementation looks really good! 👍

Regarding the test failures, we should copy out a few CoffeeScript snippets, compile them to JS, and then run that through Babel and have a look at all three. Then we need to think about reporting bugs to Babel (or V8?) and/or changing CoffeeScript's output.

@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Apr 26, 2017

So I traced down the test errors, and it was caused by the code in the nodes.coffee constructors for the Value and Op classes. Those constructors had return statements as the first lines of the constructors, that would return under certain situations; and then there was a super() call afterward. That’s apparently what Babel didn’t like.

I moved the super() calls to be the first lines of the constructors, and suddenly all the browser tests passed. The regular tests still pass, too. So I guess . . . is this an acceptable change? @connec, you’re the expert on classes, is there any reason why super() can’t or shouldn’t be the first line of the Value and Op constructors? See commit c4645a6.

Assuming this is okay, hopefully this is ready to merge. And we’ll have a browser compiler that works in . . . whatever browsers babel-preset-env supports. ES5?

@GeoffreyBooth GeoffreyBooth changed the title [WIP][CS2] Replace Closure Compiler with Babili [CS2] Replace Closure Compiler with Babili, transform browser compiler into ES5-ish Apr 26, 2017
@lydell
Copy link
Collaborator

lydell commented Apr 26, 2017

As soon as @connec has OK:ed this I can be merged.

I think these are the supported browsers: http://browserl.ist/?q=defaults

@GeoffreyBooth GeoffreyBooth requested a review from connec April 26, 2017 06:52
super()

return base if not props and base instanceof Value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor point, but this was deliberately before the super call as it's an early return so the call is redundant. Was something broken?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind, just saw the commit log!

Seems odd to me that it would fail though 😕

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What broke was just how Babel converted the classes into ES5. So as long as the only effect of this change is just to make things slightly slower, I think we’re okay. The browser compiler isn’t built for speed. The Node-based compiler will not be using a Babel-transformed version of the files, it will still be the full ES2015 version.

Copy link
Collaborator

@connec connec Apr 26, 2017

Choose a reason for hiding this comment

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

babel/babel#4854 is the upstream issue for this bug, so we're not the first to notice it at least (though it's been sitting quiet for a few months).

It's a shame to rewrite the code for the browser build, especially when it actually runs in fine in browsers that support ES2015 😢

Regardless, I don't think this should block the merge, since having broad browser support is good for the CS website, and there is an upstream issue in place 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, it’s also a good test that the workflow we’re recommending for people, of piping our compiler’s output through Babel, actually works 😄 and we can eat our own dogfood. I’d rather force that on us and live up to our recommendations rather than chicken out and assume that users of the browser compiler will all be in runtimes with support for class.

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