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

Support import and export of ES2015 modules #4300

Merged
merged 92 commits into from
Sep 14, 2016

Conversation

GeoffreyBooth
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth commented Aug 31, 2016

This pull request adds support for ES2015 modules, by recognizing import and export statements. The following syntaxes are supported, based on the MDN import and export pages:

import "module-name"
import defaultMember from "module-name"
import * as name from "module-name"
import { } from "module-name"
import { member } from "module-name"
import { member as alias } from "module-name"
import { member1, member2 as alias2,  } from "module-name"
import defaultMember, * as name from "module-name"
import defaultMember, {  } from "module-name"

export default expression
export class name
export { }
export { name }
export { name as exportedName }
export { name as default }
export { name1, name2 as exportedName2, name3 as default,  }

export * from "module-name"
export {  } from "module-name"

As a subsitute for ECMAScript’s export var name = … and export function name {}, CoffeeScript also supports:

export name = 

CoffeeScript also supports optional commas within { … }.

This PR converts the supported import and export statements into ES2015 import and export statements; it does not resolve the modules. So any CoffeeScript with import or export statements will be output as ES2015, and will need to be transpiled by another tool such as Babel before it can be used in a browser. If this PR is accepted, we will need to add a warning to the documentation explaining this.

This should be fully backward-compatible, as import and export were previously reserved keywords. No flags are used.

There are extensive tests included, though because no current JavaScript runtime supports import or export, the tests compare strings of what the compiled CoffeeScript output is against what the expected ES2015 should be. I also conducted two more elaborate tests:

  • I forked the ember-piqu project, which was an Ember CLI app that used ember-cli-coffeescript and ember-cli-coffees6 (which adds “support” for import/export by wrapping such statements in backticks before passing the result to the CoffeeScript compiler). I removed ember-cli-coffees6 and replaced the CoffeeScript compiler used in the build chain with this code, and the app built without errors. Demo here.
  • I also forked the CoffeeScript version of Meteor’s Todos example app, and replaced all of its require statements with the import and export statements from the original ES2015 version of the app on its master branch. I then updated the coffeescript Meteor package in the app to use this new code, and again the app builds without errors. Demo here.

The discussion history for this work started here and continued here. @lydell provided guidance, and @JimPanic and @rattrayalex contributed essential code.

GeoffreyBooth and others added 30 commits July 24, 2016 20:47
I couldn't get import as an expression to work, so I resorted to define it as a
statement for now.

I'm pretty sure multi-line imports don't work either and there's no alias
functionality yet. So this is still *heavily* WIP.
I think I'm getting a hang of how it's supposed to work. 🙌

What works now, as you can see in all the tests that are not commented out and
should run through:

```coffee
import foo
import bar from lib
import { foo } from lib
import { foo as boo, bar } from lib
import { foo, bar } from lib
```
… ES2015 syntax; and update tests to cover every syntax example on the Mozilla documentation pages
…bar, baz as qux }`, `export default foo = 'bar'`, `export default ->`, `export { foo as default, bar }`
@lydell
Copy link
Collaborator

lydell commented Sep 13, 2016

Why are we trying to match Babel’s AST?

Because I thought it was more or less decided over at the coffeescript6/discuss repo that a future goal is to make the compiler part of CoffeeScript simply turn its own CoffeeScript AST into Babel AST and then hand that off to Babel's stringifier (babel-generator). (Or you could pass the Babel AST directly to Babel for transforming into ES5). So my thinking has been that the closer we are to the Babel AST from the beginning, the easier we make things for us down the line.

@rattrayalex
Copy link

Hmm, I definitely leave that up to @GeoffreyBooth , but my impression is that while it might be nice to compile to a Babel AST at some point, it's not an immediate goal and not something we want to hold us back at this point.

@vendethiel
Copy link
Collaborator

Congrats on your contributor stripes, @GeoffreyBooth :).

@lydell
Copy link
Collaborator

lydell commented Sep 13, 2016

@GeoffreyBooth Thinking about the AST some more, Babel (and the other ESTree(-inspired) parsers) don't actually have the best AST here. Mixing the default binding and named imports doesn't really make sense. babel-generator has some pretty ugly code for turning that into a string again. It might make sense to go with what we have.

@GeoffreyBooth
Copy link
Collaborator Author

@vendethiel Thanks! I think the class keyword would be so much cooler renamed to klass . . .

@lydell agreed. I also don’t think we should make assumptions just yet about how we might theoretically implement a new compiler. I agree that the AST-matching approach holds a lot of promise, but until we know what exactly we’re trying to produce as nodes output I don’t think we should make our current code unnecessarily less comprehensible. I also feel like it’s likely that we take the approach of Decaf, and have the new compiler only handle certain types of nodes and fall back on the old compiler for others. In the case of modules, if the old compiler is already outputting ESNext then there’s not really a need for redoing it (until the day comes that the new compiler is ready to compile everything, and we’re eliminating the last few old compiler-generated nodes).

@vendethiel
Copy link
Collaborator

@vendethiel Thanks! I think the class keyword would be so much cooler renamed to klass . . .

Rubyists around the world agree :P.

@lydell
Copy link
Collaborator

lydell commented Sep 13, 2016

@GeoffreyBooth What do you think about this:

diff --git a/src/grammar.coffee b/src/grammar.coffee
index 93a7c0b..bf236a4 100644
--- a/src/grammar.coffee
+++ b/src/grammar.coffee
@@ -353,12 +353,12 @@ grammar =

   Import: [
     o 'IMPORT String',                                                                -> new ImportDeclaration null, $2
-    o 'IMPORT ImportDefaultSpecifier FROM String',                                    -> new ImportDeclaration [$2], $4
-    o 'IMPORT ImportNamespaceSpecifier FROM String',                                  -> new ImportDeclaration [$2], $4
-    o 'IMPORT { } FROM String',                                                       -> new ImportDeclaration [new ImportSpecifierList []], $5
-    o 'IMPORT { ImportSpecifierList OptComma } FROM String',                          -> new ImportDeclaration [new ImportSpecifierList $3], $7
-    o 'IMPORT ImportDefaultSpecifier , ImportNamespaceSpecifier FROM String',         -> new ImportDeclaration [$2, $4], $6
-    o 'IMPORT ImportDefaultSpecifier , { ImportSpecifierList OptComma } FROM String', -> new ImportDeclaration [$2, new ImportSpecifierList($5)], $9
+    o 'IMPORT ImportDefaultSpecifier FROM String',                                    -> new ImportDeclaration new ImportClause($2, null), $4
+    o 'IMPORT ImportNamespaceSpecifier FROM String',                                  -> new ImportDeclaration new ImportClause(null, $2), $4
+    o 'IMPORT { } FROM String',                                                       -> new ImportDeclaration new ImportClause(null, new ImportSpecifierList []), $5
+    o 'IMPORT { ImportSpecifierList OptComma } FROM String',                          -> new ImportDeclaration new ImportClause(null, new ImportSpecifierList $3), $7
+    o 'IMPORT ImportDefaultSpecifier , ImportNamespaceSpecifier FROM String',         -> new ImportDeclaration new ImportClause($2, $4), $6
+    o 'IMPORT ImportDefaultSpecifier , { ImportSpecifierList OptComma } FROM String', -> new ImportDeclaration new ImportClause($2, new ImportSpecifierList $5), $9
   ]

   ImportSpecifierList: [
diff --git a/src/nodes.coffee b/src/nodes.coffee
index c0ad7a3..ed928f6 100644
--- a/src/nodes.coffee
+++ b/src/nodes.coffee
@@ -1250,11 +1250,7 @@ exports.ImportDeclaration = class ImportDeclaration extends ModuleDeclaration

     code = []
     code.push @makeCode "#{@tab}import "
-
-    if @clause? and @clause.length isnt 0
-      for subclause, index in @clause
-        code.push @makeCode ', ' if index isnt 0
-        code = code.concat subclause.compileNode o
+    code.push @clause.compileNode(o)... if @clause?

     if @source?.value?
       code.push @makeCode ' from ' unless @clause is null
@@ -1263,6 +1259,24 @@ exports.ImportDeclaration = class ImportDeclaration extends ModuleDeclaration
     code.push @makeCode ';'
     code

+exports.ImportClause = class ImportClause extends Base
+  constructor: (@defaultBinding, @namedImports) ->
+
+  children: ['defaultBinding', 'namedImports']
+
+  compileNode: (o) ->
+    code = []
+
+    if @defaultBinding
+      code.push @defaultBinding.compileNode(o)...
+      code.push @makeCode ', ' if @namedImports
+
+    if @namedImports
+      code.push @namedImports.compileNode(o)...
+
+    code
+
+
 exports.ExportDeclaration = class ExportDeclaration extends ModuleDeclaration
   compileNode: (o) ->
     @checkScope o, 'export'

@GeoffreyBooth
Copy link
Collaborator Author

@lydell doesn’t that push us even farther from Babel’s AST? Wouldn’t the nodes generated by that be ImportDeclaration » ImportClause » ImportSpecifierList » ImportSpecifier?

Would we create an ExportClause too? This PR used to have ImportClause and ExportClause but I removed them because we were trying to get closer to Babel.

@lydell
Copy link
Collaborator

lydell commented Sep 13, 2016

Yes, that would move us farther away from Babel. But we kinda decided not to make the AST exactly like Babel anyway, and then I think it's nicer to have @clause be some kind of Node, rather than an array. An array doesn't really make sense, but a ImportClause with one field for "either side of the import comma" does. For export, @clause already seems to always be a Node, and doesn't really benefit from having a special ExportClause, does it?

Again, in short, if we're not going "full Babel AST" (which I have nothing against, now that I've realized that Babel's AST is worse on this particular point) that means we're going to stick with what we have instead. If we stick with what we have, we could just as well make it a tiny bit better. IMO, the code became easier to understand.

@GeoffreyBooth
Copy link
Collaborator Author

Alrighty. I added some existential operators rather than checking just for truthiness, but otherwise it’s exactly your diff. code.push @clause.compileNode(o)... if @clause? is pretty slick.

With regard to the AST, let’s leave that for when we actually start implementing that approach. I have a feeling we’re going to have some kind of intermediate layer where we rewrite our AST tree to conform to the target tree. (Doesn’t Decaffeinate have something like that?) A rewrite layer might be preferable to awful contortions in our grammar. Anyway, let’s deal with it later when we’ll have a better sense of which is the less-bad option.

Anything else?

@lydell
Copy link
Collaborator

lydell commented Sep 14, 2016

I have a feeling we’re going to have some kind of intermediate layer where we rewrite our AST tree to conform to the target tree. (Doesn’t Decaffeinate have something like that?) A rewrite layer might be preferable to awful contortions in our grammar.

Correct! Since CoffeeScript isn't JavaScript, it is going to need to have its own AST nodes for many things. The parsing phase builds the AST from a string. The compiling phase takes the AST as input, and outputs JavaScript AST. Another tool can then turn that JS AST into a string.

So, you're right. There's no need to match Babel exactly here – we won't be able to in many places anyway. I think we've landed on a very nice spot right now – our AST is not unnecessarily different from Babel's, and it's better where it makes sense for us.

With regard to the AST, let’s leave that for when we actually start implementing that approach. … let’s deal with it later when we’ll have a better sense of which is the less-bad option.

Definitely! 👍

Anything else?

I don't think so. We'll probably merge tonight.

@lydell lydell merged commit 66ac8af into jashkenas:master Sep 14, 2016
@lydell
Copy link
Collaborator

lydell commented Sep 14, 2016

There we go, merged! Now, I think we should:

  1. Document import/export.
  2. Make a new release. See CoffeeScript 1.10.0 #4079 for inspiration.

@JimPanic
Copy link
Contributor

Amazing! 🙌

@GeoffreyBooth
Copy link
Collaborator Author

Thank you @lydell! Should I start a new branch for the documentation? On my repo or the main one?

If I handle the docs do you want to handle the release? 😄

@GeoffreyBooth GeoffreyBooth deleted the import-export branch September 14, 2016 19:13
@lydell
Copy link
Collaborator

lydell commented Sep 14, 2016

Should I start a new branch for the documentation? On my repo or the main one?

I usually do stuff in branches on my own fork and then submit a pull request.

If I handle the docs do you want to handle the release? 😄

Ok.

@jashkenas
Copy link
Owner

There we go, merged! Now, I think we should:

Document import/export.
Make a new release. See #4079 for inspiration.

Fantastic. @lydell, you're now an owner of the npm module as well, so feel free to release when you're ready.

@vendethiel
Copy link
Collaborator

👍

constructor: ->
console.log 'hello, world!'
''', '''
SyntaxError: Unexpected token export
Copy link

Choose a reason for hiding this comment

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

I think this test does not really check what it says it does. "Unexpected token export" is an error from Node.js because it does not support ES6 modules. So the code under test actually compiles to JavaScript and then fails on when tried to run. It should fail during coffee compilation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dammit, you’re completely correct. The generated JavaScript for that test is:

export var (function() {
  function _Class() {
    console.log('hello, world!');
  }

  return _Class;

})();

which is completely invalid. I’ll add a check to catch this within the CoffeeScript compiler. Thanks for pointing this out.

Copy link

Choose a reason for hiding this comment

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

Thanks! I'm glad I was able to help :)

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.

10 participants