pahen / madge

Create graphs from your CommonJS, AMD or ES6 module dependencies
MIT License
9.09k stars 318 forks source link

ES6 tests failing #61

Closed tafsiri closed 8 years ago

tafsiri commented 9 years ago

I was trying to analyse dependencies written in es6 syntax and had some trouble and then discovered that the es6 related tests are failing. This is on master sha 3b4957bafbb12d3626d0c8412a5bdd52232e8ce5

after checkout out the repo I ran npm install and npm test and got the following output.

  module format (ES6)
    1) should behave as expected on ok files
    ✓ should tackle errors in files 
    2) should be able to exclude modules
    3) should find circular dependencies
    4) should find absolute imports from the root

...

  1) module format (ES6) should behave as expected on ok files:

      AssertionError: expected Object {
  a: Array [],
  d: Array [],
  'fancy-main/not-index': Array [],
  'sub/b': Array [],
  'sub/c': Array []
} to equal Object {
  a: Array [ 'sub/b' ],
  d: Array [],
  'fancy-main/not-index': Array [],
  'sub/b': Array [ 'sub/c' ],
  'sub/c': Array [ 'd' ]
} (at a -> length, A has 0 and B has 1)
      + expected - actual

       {
      +  "a": [
      +    "sub/b"
      +  ],
      -  "a": [],
         "d": [],
         "fancy-main/not-index": [],
      +  "sub/b": [
      +    "sub/c"
      +  ],
      +  "sub/c": [
      +    "d"
      +  ]
      -  "sub/b": [],
      -  "sub/c": []
       }

(there is more output, but i figure this should be easy to reproduce). Thanks

rafayepes commented 9 years ago

Why not use Babel to parse the files? Getting some issues with ES6 as well.

pahen commented 9 years ago

What version of Node.js are you running and on what platform?

rafayepes commented 9 years ago

I'm using grunt-madge v0.0.6 in Node v0.12.2.

I'm getting parsing errors with things like arrow functions, for..Of, template strings,...

"Error while parsing file: foo.js" "Warning: Line 13: Unexpected token =>"

I have no issues using let and const.

EDIT: I can run all tests in green. If I change the es6 examples to use arrow functions, they still work. However, if I change the AMD examples to use arrow functions they break.

1) module format (AMD) should resolve relative module indentifiers:

      AssertionError: expected Object {
  a: Array [],
  b: Array [],
  'foo/bar/d': Array [ 'a' ],
  'foo/c': Array [ 'a' ]
} to equal Object {
  a: Array [],
  b: Array [ 'a' ],
  'foo/bar/d': Array [ 'a' ],
  'foo/c': Array [ 'a' ]
} (at b -> length, A has 0 and B has 1)
      + expected - actual

       {
         "a": []
      -  "b": []
      +  "b": [
      +    "a"
      +  ]
         "foo/bar/d": [
           "a"
         ]
         "foo/c": [

      at Assertion.fail (node_modules/should/lib/assertion.js:180:17)
      at Assertion.prop.value (node_modules/should/lib/assertion.js:65:17)
      at Context.<anonymous> (test/amd.js:105:19)

Might it be that when using AMD, there is a different parser being use or something like that? My project uses AMD for module loading but uses other ES6 features. I believe amdetective doesn't support ES6 features.

rafayepes commented 9 years ago

I believe my issues with AMD and ES6 this should be fixed by mixu/amdetective#3

rafayepes commented 9 years ago

Can we update amdetective dependency to v0.1.0? Would be nice to update grunt-madge as well to include ES6 support for AMD modules as well.

Thanks!

mrjoelkemp commented 9 years ago

I have an alternate detective for amd that also has es6 and jsx support (uses acorn under the hood): https://github.com/mrjoelkemp/node-detective-amd. In addition, it accepts as AST (or a file's contents) to avoid double parsing. Happy to help integrate it into madge.

mrjoelkemp commented 9 years ago

Btw, madge is also using my detective-es6 plugin which also supports jsx and an AST source. Just as an fyi, I wrote precinct: https://github.com/mrjoelkemp/node-precinct to avoid worrying about module types and get the dependencies of any JS file (es6, cjs, amd) with an AST or file content source. It also supports SASS/Stylus (Less would be easy to add). It's a detective factory. May remove a lot of logic from madge core.

deanrad commented 8 years ago

@mrjoelkemp - Precinct === Detective Factory - that's hilarious. I'm stuck on JSX right now—I'll tie my work to this issue, and see if using precinct (at least for JSX) helps..

mrjoelkemp commented 8 years ago

@deanius JSX with ES6? Try updating the detective-es6 version in madge to 1.1.3 since 1.1.2 didn't have JSX support. Let me know if that works.

deanrad commented 8 years ago

@mrjoelkemp - any thoughts on how to do this with files that need babel precompilation? I've got react files that use object-rest-spread babel plugin and they break madge.

rafayepes commented 8 years ago

@deanius you should probably compile those files with Babel first. Then, point madge to the folder were your Babel output files are. That should do the trick.

mrjoelkemp commented 8 years ago

Detective-es6 could benefit from using babylon (babel's parser) instead of Acorn directly for parsing files that use features at various acceptance stages.

This would be a breaking (but I'm definitely open to it) change in node-source-walk. After that, we'd push the version bump to detective-es6 and then bump the version of detective-es6 in madge.

Another option is to have node-source-walk accept a configurable parser (with acorn as the default). This seems unnecessary since babylon is a superset of Acorn (if I remember correctly).

For now, precompiling your code is a work around.

Ref https://github.com/mrjoelkemp/node-source-walk/issues/14

deanrad commented 8 years ago

Thanks @rafayepes @mrjoelkemp

mrjoelkemp commented 8 years ago

I made the fix to detective-es6 (more specifically to node-source-walk) to support all available ES7 plugins: https://github.com/babel/babylon#plugins. The goal is to always be able to parse a JS file for the dependencies – regardless of the syntax.

This should be fixed when https://github.com/pahen/madge/pull/83 is merged.