Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

import(cjs) messes with module.parent #469

Closed
aduh95 opened this issue Jan 15, 2020 · 29 comments
Closed

import(cjs) messes with module.parent #469

aduh95 opened this issue Jan 15, 2020 · 29 comments

Comments

@aduh95
Copy link

aduh95 commented Jan 15, 2020

Some packages use a check on module.parent to know if they are launched from CLI or required by another module:

// module.cjs
if (!module.parent) throw new Error("Running from CLI is not supported!");

This works fine if you use require (I tried from both CJS and ES modules):

$ node -p "assert.doesNotThrow(()=>require('./module.cjs')), 'No error!';"
No error!

However, when importing from ESM, that doesn't work:

// module.mjs
import './module.cjs'; // This will throw the "don't run from CLI" Error...
console.log('This is fine.'); // This is never reached

Same for dynamic imports:

// module.js // (same behaviour on ESM and CJS)
import('./module.cjs')
  .then(()=>console.log('This is fine.'))
  .catch(()=>console.log('Oh no!'))

I can see three ways to tackle this issue, but none is very satisfying to me:

  • Make module.parent truthy when a CJS is imported (although I have no idea what it could look like).
  • Deprecate module.parent? Packages using require.main===module are fine, so that's what library authors should be using to test this, right?
  • Disregard this issue, as users can use module.createRequire to workaround the issue.
@jkrems
Copy link
Contributor

jkrems commented Jan 15, 2020

Thanks for bringing this up, I don't think we were tracking this yet! Your options sound spot-on.

Make module.parent truthy when a CJS is imported (although I have no idea what it could look like).

Making module.parent could be tricky if any package actually tries to check it - it would have to be something that itself isn't a valid CJS module. So we'd have to select a value very carefully or risk breaking other uses of module.parent if those exist.

Packages using require.main===module are fine, so that's what library authors should be using to test this, right?

Yeah, I think to me require.main===module is the "canonical" check. That's also what the docs suggest using: https://nodejs.org/api/modules.html#modules_accessing_the_main_module. So for maintainers, the suggested path would be to migrate to that pattern instead.

Disregard this issue, as users can use module.createRequire to workaround the issue.

I think it'll depend on how widespread this issue is. Can you share which package (or packages) this happened with? Capturing a list here may make it easier for us to determine the impact.

@SimenB
Copy link
Member

SimenB commented Jan 15, 2020

One use case for module.parent, fwiw: https://github.com/hypercloud/import-dir/blob/afb1ef9ae1849e6ce8b535f69d412e38dda3115b/index.js#L14. Not a highly used module, but still

@bmeck
Copy link
Member

bmeck commented Jan 15, 2020

@SimenB not all ESM have a valid file path associated with them (data: in particular for now), and/or there may have multiple ESM with the same file path but different URLs (using URL fragments). I'm not sure we could preserve that through populating module.parent from an ESM base.

@aduh95
Copy link
Author

aduh95 commented Jan 15, 2020

For reference, I have seen is-port-available and pdf-parse use a similar pattern. Not highly used modules either, but there are certainly others.

@ExE-Boss
Copy link

ExE-Boss commented Feb 1, 2020

I think we should deprecate module.parent.

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Mar 12, 2020

Folks, I'm very much opposed to deprecating module.parent.

In fact, I'm in need of this feature to be functional for ES modules too.

The intended use-case is to be able to visualize module graphs. To be able to traverse these graphs, I'd need this parent-children relationship metadata. This also relates to APMs.

Additionally, I do not think something so important should be deprecated merely to discourage a pattern.

@devsnek
Copy link
Member

devsnek commented Mar 12, 2020

@DerekNonGeneric parent does not give an accurate traversal of a graph anyway, because modules can actually have multiple parents. you should start at the entrypoint and traverse down.

@DerekNonGeneric
Copy link
Contributor

I'm sorry, can you clarify? I'm aware CJS and ES modules use different algorithms, but module.parent shows undefined in my .mjs graphs anyways, so I was referring to CJS graphs. I'd appreciate a link to spec docs if possible.

@devsnek
Copy link
Member

devsnek commented Mar 12, 2020

@DerekNonGeneric like require('x.js') in a.js and require('x.js') in b.js. Both a.js and b.js are parents of x.js, but only the one that ran first would be the module.parent.

@DerekNonGeneric
Copy link
Contributor

That's perfectly fine! I'm interested in visualizing the graph as it exists during execution (not conceptually).

@devsnek
Copy link
Member

devsnek commented Mar 12, 2020

@DerekNonGeneric ... during execution both a.js and b.js required x.js. a cjs module graph would show a directed edge from a.js to x.js and a directed edge from b.js to x.js.

@DerekNonGeneric
Copy link
Contributor

I'm unclear why this information would be useful to a stack trace, but unavailable to userland. I'd have to do more research, but this seems like valuable information is being lost.

@devsnek
Copy link
Member

devsnek commented Mar 12, 2020

@DerekNonGeneric the graph structure is preserved (in a more accurate manner) via module.children.

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Mar 12, 2020

@devsnek, thank you. I don't feel the ends justify the means here. It seems like the correct way to go about this would be to deprecate the module that's using the discouraged pattern, not deprecate a runtime feature because people are using it incorrectly.

@devsnek
Copy link
Member

devsnek commented Mar 12, 2020

@DerekNonGeneric my point is that there is no "correct" way to use module.parent. At best it seems like you want module.parents, but we don't have that and module.parent is not that.

@DerekNonGeneric
Copy link
Contributor

This would also prevent building stack trace libraries too, though wouldn't it? Perhaps a good compromise would be to rename it to something else as opposed to deprecation?

@devsnek
Copy link
Member

devsnek commented Mar 12, 2020

@DerekNonGeneric what does module.parent have to do with stack traces?

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Mar 12, 2020

I was under the impression that one would be able to use a cache (that hasn't been tampered with) to determine the execution order.

From my perspective, what would justify deprecation would be if the contents of module.parent are generally bogus and serve no purpose whatsoever. If that's the case, then I stand corrected.

(It's not just a type, it's actually a fully-populated and observable module record-like object.)

@devsnek
Copy link
Member

devsnek commented Mar 12, 2020

@DerekNonGeneric it tells you what the first module to require a module is, but that doesn't allow you to discern the execution order of the graph as a whole. Object.keys(require.cache) is ordered by insertion though.

@bmeck
Copy link
Member

bmeck commented Mar 12, 2020

Reading the use case, we can make that an issue rather than a side effect from the existence of module.parent. As @devsnek points out there are problems with module.parent and we could better serve this use case if we actually fleshed out needs and discussed what needs to be done. I'd prefer we open a separate issue about tracking graph relationships in a way that works in a forwards compatible manner and solves some of the existing problems.

@DerekNonGeneric
Copy link
Contributor

@bmeck, good plan, will do.

@DerekNonGeneric
Copy link
Contributor

I happened to stumble upon a published tip (post) with a legitimate use of module.parent.
This use case was also mentioned in OP above as well. I've inlined it here for safe keeping.


In node, you can tell your program to do two different things depending on whether the code is run from require('./something.js') or node something.js. This is useful if you want to interact with one of your modules independently.

if (!module.parent) {
    // ran with `node something.js`
    app.listen(8088, function() {
        console.log('app listening on port 8088');
    })
} else {
    // used with `require('/.something.js')`
    module.exports = app;
}

I'm unsure we have a viable replacement for this in ES module context.
If we do, it does not appear to be documented (maybe @aduh95's PR).

I would hope this wouldn't involve createRequire in the future.

@devsnek
Copy link
Member

devsnek commented May 6, 2020

That's part of a separate discussion about how and if entries should be classified: nodejs/node#49440

@DerekNonGeneric
Copy link
Contributor

@devsnek, they're certainly very similar, but according to Guy in nodejs/node#49440:

Dynamic import does not affect this, as what we are distinguishing is the CLI entry point, not the module graph entry point.

That issue seems to be about CLI vs package entry points (main).

I would be interested in knowing the module graph entry point (no ancestors / root node).

@devsnek
Copy link
Member

devsnek commented May 6, 2020

that's somewhat purposely not a thing in esm. in any case this issue is about module.parent which is being deprecated so i'm going to close it out.

@devsnek devsnek closed this as completed May 6, 2020
@DerekNonGeneric
Copy link
Contributor

@aduh95, your PR is recommending to use …

if (require.main === module) {
}

… as opposed to …

if (!module.parent) {
}

These may appear to achieve the same desired effect, but that's not the case in
every circumstance. The following points explain why I'm still not +1 and might
be worth considering.

  • Because require can be “hijacked” (and often is), none of its behavior can
    be trusted with any degree of certainty. A good example is
    node -r esm.
  • Because module is a “free variable” in the global CommonJS scope, it never
    gets loaded with require(), which may provide more certainty that its
    contents are trustworthy (since require() never had the opportunity to
    tamper w/ it).

Therefore, if I'm not mistaken, module.parent is currently the only credible
way to determine whether or not the current module is the root node of the
module graph.


Ultimately, I'm not attached to the outcome of the deprecation PR since it's
just documentation deprecation (for now). However, if the docs deprecation were
to eventually become runtime deprecation, it seems to me like it would come at
the expense of module graph observability, which doesn't seem like the intention.

@jkrems
Copy link
Contributor

jkrems commented May 6, 2020

Therefore, if I'm not mistaken, module.parent is currently the only credible
way to determine whether or not the current module is the root node of the
module graph.

It's not any more or less credible than every other option. There's no reliability difference (afaik) between require.main and module.parent. Both can be influenced by code running before the module because CommonJS is highly hackable. module.parent is very easy to change by just using Module._load:

$ cat /tmp/my.cjs
console.log(module.parent ? "has parent" : "no parent")
$ node /tmp/my.cjs
no parent
$ node -e "require('/tmp/my.cjs')"
has parent
$ node -e "require('module')._load('/tmp/my.cjs', null, true)"
no parent

You can even modify require.cache['/realpath/to/my.cjs'].parent after the fact and make the parent appear and disappear at runtime.

it seems to me like it would come at the expense of module graph observability

I don't agree since require.cache never modeled a full or reliable module graph. And if we ever bring a module graph to ESM in general, it would likely not be in the CommonJS cache. So require.cache will become less and less complete one way or another as people start using ESM features.

@aduh95
Copy link
Author

aduh95 commented May 6, 2020

Therefore, if I'm not mistaken, module.parent is currently the only credible
way to determine whether or not the current module is the root node of the
module graph.

I believe process.mainModule achieves the same purpose as well. It's been doc deprecated on v14.0.0 but it's still an alternative.

Because require can be “hijacked” (and often is), none of its behavior can
be trusted

That doesn't seem like a problem we would want to fix, I guess people that do "hijack" require do it for their own reasons. If you want certainty, you shouldn't use code that "hijacks" it, wouldn't you agree?

Anyway, module.parent has other issues that have been discussed in this thread already, I'm not sure further discussion can be constructive at this point.

aduh95 added a commit to aduh95/node that referenced this issue May 20, 2020
This feature does not work when a module is imported using ECMAScript modules
specification, therefore it is deprecated.

Fixes: nodejs/modules#469
BridgeAR pushed a commit to BridgeAR/node that referenced this issue May 24, 2020
This feature does not work when a module is imported using ECMAScript
modules specification, therefore it is deprecated.

Fixes: nodejs/modules#469

PR-URL: nodejs#32217
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
aduh95 added a commit to aduh95/node that referenced this issue Jun 9, 2020
This feature does not work when a module is imported using ECMAScript
modules specification, therefore it is deprecated.

Fixes: nodejs/modules#469

PR-URL: nodejs#32217
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 15, 2020
This feature does not work when a module is imported using ECMAScript
modules specification, therefore it is deprecated.

Fixes: nodejs/modules#469

Backport-PR-URL: #33533
PR-URL: #32217
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Jul 16, 2020
This feature does not work when a module is imported using ECMAScript
modules specification, therefore it is deprecated.

Fixes: nodejs/modules#469

Backport-PR-URL: #33533
PR-URL: #32217
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
aduh95 added a commit to aduh95/node that referenced this issue Aug 18, 2020
This feature does not work when a module is imported using ECMAScript
modules specification, therefore it is deprecated.

Fixes: nodejs/modules#469

PR-URL: nodejs#32217
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this issue Aug 18, 2020
This feature does not work when a module is imported using ECMAScript
modules specification, therefore it is deprecated.

Fixes: nodejs/modules#469

PR-URL: #32217
Backport-PR-URL: #34592
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants