ES6 module import tests relies on a version of esprima that doesn't follow estree spec

[fczuardi]

If you look at https://github.com/estools/escope/blob/master/test/es6-import.coffee#L25 you see that the tests for es6 uses an esprima version that does not generate https://github.com/estree/estree spec compliant ast

For example:

ast.body[0].specifiers for code 'import v from "mod"' is expected to output:

 [ { type: 'ImportDefaultSpecifier', local: { type: 'Identifier', name: 'v' } } ]

and not

 [ { type: 'ImportDefaultSpecifier', id: { type: 'Identifier', name: 'v' } } ]

[fczuardi]

by the way, there is no version of esprima at the moment that follows estree, the harmony branch is old and master is expected to merge https://github.com/eslint/espree changes soon according to comments on Freenode's IRC channel #esprima

[fczuardi]

related: #33

[fczuardi]

and the proper esprima issue might be this one: jquery/esprima#1000

[nzakas]

Yeah, Espree is the first parser to implement the ESTree representation of ES6 modules.

[Constellation]

estraverse, esrecurse are now upgraded to suppert those types :)
So next is escope :D

[nzakas]

Yay!

[fczuardi]

if replaced with espree, lines such as this one https://github.com/estools/escope/blob/master/src/referencer.js#L117 must be updated to use node.local instead of node.id

[fczuardi]

one possible fix to this bug:

diff --git a/package.json b/package.json
index f9ea500..9b45783 100644
--- a/package.json
+++ b/package.json
@@ -28,6 +28,7 @@
     "browserify": "^9.0.3",
     "chai": "^2.1.1",
     "coffee-script": "^1.9.1",
+    "espree": "^1.11.0",
     "esprima": "~1.2.2",
     "gulp": "~3.8.10",
     "gulp-babel": "^4.0.0",
diff --git a/src/referencer.js b/src/referencer.js
index 7b592ce..a237e43 100644
--- a/src/referencer.js
+++ b/src/referencer.js
@@ -108,20 +108,20 @@ class Importer extends esrecurse.Visitor {
     }

     ImportNamespaceSpecifier(node) {
-        if (node.id) {
-            this.visitImport(node.id, node);
+        if (node.local) {
+            this.visitImport(node.local, node);
         }
     }

     ImportDefaultSpecifier(node) {
-        this.visitImport(node.id, node);
+        this.visitImport(node.local, node);
     }

     ImportSpecifier(node) {
         if (node.name) {
             this.visitImport(node.name, node);
         } else {
-            this.visitImport(node.id, node);
+            this.visitImport(node.local, node);
         }
     }
 }
diff --git a/test/es6-import.coffee b/test/es6-import.coffee
index 11d8a6b..6e524b3 100644
--- a/test/es6-import.coffee
+++ b/test/es6-import.coffee
@@ -22,7 +22,7 @@
 #  THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

 expect = require('chai').expect
-harmony = require '../third_party/esprima'
+harmony = require 'espree'
 escope = require '..'

 describe 'import declaration', ->

[nzakas]

It would probably be easier to replace node.id with node.local || node.id

[fczuardi]

true, here is a pull request: #52

[Constellation]

Other nodes, (exportXXX should be handled).

[ai]

@Constellation do you have a plan to release this fix?

[Constellation]

@nzakas, @ai

What do you think of updating the major number with these ES6 commits?

[ai]

@Constellation it will be more difficult for me personally, because I will be wait for eslint update (this issue is critical for me).

But we must do it, if we change the API. But maybe it is only a fix, because new behaviour was expected from current API?

[nzakas]

It looks like these changes are backwards compatible, so I don't think a major version bump is necessary.

[Constellation]

@nzakas, @ai

Ah, my concern is that, to align to estree that is actively developed, we will need to change/add/drop many nodes. So I'm worrying about that backporting becomes difficult.
Maybe, this change can be easily ported to 2.0.x. So I'll pick it to 2.0.x now :)

[nzakas]

Oh yeah, I was just thinking about the changes for this issue, specifically. With all the node changes, it's probably best to do a major version bump.

[Constellation]

And I'll pick super support and I'll publish it as 2.0.7 soon! :D

[nzakas]

Awesome!

[Constellation]

Released as 2.0.7 :)

[nzakas]

Thanks!

[nzakas]

@Constellation I'm not sure what happened, but 2.0.7 seems to have a regression related to AssignmentPattern. See: eslint/eslint#2001

[Constellation]

@nzakas

Ah, current escope is locked with estraverse 1.9.x because there's no exportxxx node support in estree.
We need to implement all ExportXXX node before updating estraverse to 2.0.x.

[Constellation]

@nzakas

Maybe, to support ExportXXX, only modifying ExportSpecifier is sufficient I think.
I'll create a patch and update the estraverse contained in escope.

[Constellation]

AssignmentPattern is done in b713c53.
This is initial support of AssignmentPattern and TDZ scope issue is not fixed yet.