top-level "define" results in false positives for "amd" type

[boneskull]

I think this issue belongs here, but might belong in module-definition.

I found a false positive with ansi-colors, which has a top-level define, but it (ahem) defines define itself. module-definition thinks this is an AMD module.

I'm not sure how this should be addressed. Should module-definition only consider a module to be AMD if define is used as a global? Or if define has a certain specific signature? Unsure if the change should live in module-definition or here or both.

If you can point me in the right direction, I may be able to send a PR to address it.

thanks!

[mrjoelkemp]

Hey man! A familiar face :) Thanks for the the issue.

This might as well have been written by someone else it's been so long :D

Just so I know which layer you're working at, are you using https://github.com/dependents/node-dependency-tree or a lower-level dependency directly?

I went back and forth on approaches here since, to your point, there are a few ways of fixing this. I originally thought about redefining isDefine here, but that's technically a breaking change since it would restrict cases that previously passed and I'd have to patch things up as bug reports come in.

I think a less invasive fix is to expose a new isDefineAMD in this module that does something like:

module.exports.isDefineAMD = function (node) {
  if (!node) return false;

  return isNamedForm(node) || isDependencyForm(node) || isFactoryForm(node) || isNoDependencyForm(node) || isREMForm(node);
};

and then patch module-definition here and detective-amd here to use that more specific method.

What do you think?

[boneskull]

I was using precinct.

The actual exception I encountered was--upon recognition of ansi-colors as an AMD module--failure to parse the AST (since, presumably, it's not of the expected module type). If anything would be helpful there, it'd probably be wrapping the error (it was Unknown node type: NumericLiteral coming out of escodegen via detective-amd) in something contextual, e.g., "failed to parse ansi-colors as an AMD module"

Do you need a hand?

[boneskull]

maybe not "failure to parse the AST", actually, since I have no idea what escodegen is used for here 🙂

[boneskull]

as long as I'm here: what I'm using precinct (alongside filing-cabinet in some cases) to do is:

determine (via static analysis) a list of test files to rerun if a source file changes when running mocha in "watch" mode. other test frameworks leverage istanbul for this, but it's not appropriate with mocha's architecture.

if there's another lib you think I should look at, please let me know. 🙂

[mrjoelkemp]

I'm working on the addition of isDefineAMD now. Thanks for offering the hand.

I think dependency-tree: https://github.com/dependents/node-dependency-tree does what you want already. It uses both precinct and filing cabinet to generate the dependency tree from a starting file. It might work for your needs.

[mrjoelkemp]

Still getting that ansi-colors code to register as amd in module-definition even with the fix locally. Will continue looking into why.

[mrjoelkemp]

Alrighty, so fixes have been applied to both this module and module-definition, most importantly. There are tests in module-definition that confirm that the module type is not recognized as AMD.

Thanks for this issue, it pointed out a lack of rigor in AMD checking.

Since it's a patch version (technically false positive bug fixes), you should be able to reinstall cabinet and get the fix immediately. Let me know if you encounter any troubles. Thanks again.

[boneskull]

thanks!

there was some reason I wasn't using node-dependency-tree. I think it was the "tree" part--needed a different data structure. essentially a mapping where I could quickly query any file for its dependencies, regardless of its depth in the tree.