dependents / node-dependency-tree

Get the dependency tree of a module
MIT License
707 stars 84 forks source link

Correctly resolve paths on Windows by updating filing-cabinet dependency #107

Closed TimvdEijnden closed 5 years ago

TimvdEijnden commented 5 years ago

This fixes an issue where dependency's would not be resolved on Windows.

See browserify/resolve#156 (comment)

@mrjoelkemp sorry for bothering you again, but this repo needs a version bump for filing-cabinet.

TimvdEijnden commented 5 years ago

@mrjoelkemp I've investigated the failing tests, here are my findings.

The tests are failing on Node 6 & 7, this is related to ast-module-types package being used by filing-cabinet

> nvm use 6
> rm -rf node_modules/ && npm i
> node -v
    6.17.1
> npm list ast-module-types
    dependency-tree@7.0.2 /Users/tim/git/node-dependency-tree
    └─┬ filing-cabinet@2.3.3
    └─┬ module-definition@3.2.0
        └── ast-module-types@2.5.0
> npm test
    53 passing (1s)
    1 failing 

-----

> nvm use 8
> rm -rf node_modules/ && npm i
> node -v
    8.16.0
> npm list ast-module-types
    dependency-tree@7.0.2 /Users/tim/git/node-dependency-tree
    ├─┬ filing-cabinet@2.3.3
    │ └─┬ module-definition@3.1.0
    │   └── ast-module-types@2.4.0
    └─┬ precinct@6.1.1
    ├─┬ detective-amd@3.0.0
    │ ├── ast-module-types@2.4.0  deduped
    │ └─┬ get-amd-module-type@3.0.0
    │   └── ast-module-types@2.4.0  deduped
    └─┬ detective-cjs@3.1.1
        └── ast-module-types@2.4.0  deduped
> npm test
    54 passing (685ms)

There is a function named isES6Import, which is different between node version 8 and 6&7 in ast-module-types.

In node 8 it looks like this:

module.exports.isES6Import = function(node) {
  switch(node.type) {
    case 'ImportDeclaration':
    case 'ImportDefaultSpecifier':
    case 'ImportNamespaceSpecifier':
      return true;
  };

  return false;
};

In node 6&7 it look like this:

module.exports.isES6Import = function(node) {
  switch(node.type) {
    case 'Import':                                         <---------------------- is added so type is now set to es6
    case 'ImportDeclaration':
    case 'ImportDefaultSpecifier':
    case 'ImportNamespaceSpecifier':
      return true;
  };

  return false;
};

Because it's missing the Import case when using node 8 the type is not set to es6 and the detectiveEs6 is not used in precinct. Thus the dependency is found in node 6 and node 7, but not in node 8.

TimvdEijnden commented 5 years ago

Issue has been fixed by doing an npm update

TimvdEijnden commented 5 years ago

Please ignore this PR, not needed as resolve depedency is now correctly updated in filing-cabinet. After testing and updating madge the issue is resolved for us.