require.main.require not being parsed about node-precinct

[mrjoelkemp]

Hey @choxnox! Thanks for contributing.

This is similar to the following request: #42.

I'm totally open to a PR that enhances the functionality for legal module syntax. I don't have time to do this myself, but happy to answer questions and review PRs.

[choxnox]

Because this issue bothers me quite a lot (currently I have to manually include modules inside Serverless config file which is super awkward) I will actually try to "fix" this and send PR. So if I understood referenced issue correctly it's node-detective-cjs module that should be modified to support syntax from this issue?

[mrjoelkemp]

I appreciate the help. Thanks in advance for the contribution.

So if I understood referenced issue correctly it's node-detective-cjs module that should be modified to support syntax from this issue?

Yes. My assumption here is that require.main.require is a commonjs/nodejs module resolution feature. If that's the case, then node-detective-cjs contains all of the commonjs-specific logic for how to find dependencies within those types of modules.

[choxnox]

I took a quick look at the node-detective-cjs code and it seemed the problem is not there but in ast-module-types module and its isRequire function which is called from node-detective-cjs module.

So I went there and eventually made it work (in tests) by modifying that function (AST explorer has been of great help). Modules required with require.main.require get detected properly. Should I submit PR there instead?

Anyway, the only difference is this before:

// Whether or not the node represents a require function call
module.exports.isRequire = function (node) {
  if (!node) return false;

  var c = node.callee;

  return c &&
        node.type  === 'CallExpression' &&
        c.type     === 'Identifier' &&
        c.name     === 'require';
};

and after:

// Whether or not the node represents a require function call
module.exports.isRequire = function (node) {
  if (!node) return false;

  var c = node.callee;

  return c &&
        node.type  === 'CallExpression' && ((
            c.type     === 'Identifier' &&
            c.name     === 'require') || (
            c.type     === 'MemberExpression' &&
            c.object.type === 'MemberExpression' &&
            c.object.object.type === 'Identifier' &&
            c.object.object.name === 'require' &&
            c.object.property.type === 'Identifier' &&
            c.object.property.name === 'main' &&
            c.property.type === 'Identifier' &&
            c.property.name === 'require'
        ));
};

[mrjoelkemp]

Great work tracking down a fix. That's actually a better place for it since module-definition will also properly correlate that special require with commonjs.

Consider using extracting the addition to a method like:

return isPlainRequire(node) || isMainScopedRequire(node);

where isMainScopedRequire contains your new logic extracted to a method. If you could extract the original logic for sniffing require to isPlainRequire, that might help the readability of the method.

[choxnox]

Yeah, this was just a "hotfix" to see if idea sticks (tests passed). I'll go with your suggestions and submit the PR (most probably tomorrow).

[choxnox]

I finally managed to find some free time today to do this. I've just sent PR for this.

[mrjoelkemp]

@choxnox thanks again for your contribution. I pushed the support all the way up to precinct.

There's another missing piece to this though. We now have the ability to extract dependencies from main scoped require statements, but we don't yet know how to resolve them to a file on the filesystem. This is the module and code that's used to handle a commonjs style of module resolution: https://github.com/dependents/node-filing-cabinet/blob/master/index.js#L195.

Since require.main.require influences the behavior of how the module/partial is resolved, we either get it for free with our use of https://github.com/browserify/resolve here: https://github.com/dependents/node-filing-cabinet/blob/a11f774a6ef95cce8db1e700aebf45e4ee54affb/index.js#L221, or we'll have to either get that supported in that library or within filing-cabinet directly.

Does the fix in precinct completely satisfy your need, or are you trying to generate a dependency tree from your project (at which point, you'll need the mapping of module to file)? Are you open to a PR that adds main scoped commonjs module resolution support to filing cabinet?