Skip to content

module: make module.parent truthy when loaded from ESM #37702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 10, 2021

This is to ensure backward compatibility of CJS modules checking the
boolean value of module.parent to test if the module is the entry
point of the current process.

Currently, module.parent provides a way for CJS users to get which CJS modules required the current module first. This feature – not very useful by itself – is also (wrongly) used to make assumption about the program entry point. Some packages wrap their test code inside a if(!module.parent) condition, assuming that a falsy value means the module is run from CLI. The issue with this pattern is that the assumption doesn't work as soon as the module interacts with ESM or REPL.

The issue I'd like to address is when a dependency is using the if(!module.parent) pattern to run its tests, and the test code gets executed for ESM users without their knowledge; those tests that may have side effects and make the end user code fail without obvious reason.
The current behavior where module.parent is undefined therefore falsy impacts end users trying to migrate from CJS to ESM, not the package author using such pattern in one of their abandoned project. This PR switches to a dummy object which makes it truthy.

Refs: #33534

This is to ensure backward compatibility of CJS modules checking the
boolean value of `module.parent` to test if the module is the entry
point of the current process.
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 10, 2021
@aduh95 aduh95 added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 10, 2021
@targos
Copy link
Member

targos commented Mar 11, 2021

Can you share an example of a package that does it?

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 11, 2021

I encountered that with pdf-parse, I don't know how rare is it – the fact that we haven't receive any report except mine seems to indicate it's not a very spread out issue.
But it's really frustrating to stumble upon this, I'd bet most users who try out switching to ESM and use a package that uses this kind of pattern would simply go back to CJS.

@ljharb
Copy link
Member

ljharb commented Mar 11, 2021

pdf-parse now seems to use module.parent as "debug mode" and no longer throws: https://gitlab.com/autokent/pdf-parse/-/blob/master/index.js#L6

does that resolve your original issue?

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 11, 2021

does that resolve your original issue?

Hum, no the issue is still there: https://gitlab.com/autokent/pdf-parse/-/issues/15. But if I'm opening this PR, it's not to fix my old issue with that package, it's to address the more general point: do we want to support this pattern, or do we prefer to keep the current broken behavior? As I said in the OP and the other PR, I think the current behaviour only harms users that are willing to try out ESM, and I'd like to improve that.

@ljharb
Copy link
Member

ljharb commented Mar 11, 2021

I don't think it only does that - it also reinforces that this pattern shouldn't be used. It's unfortunate that https://gitlab.com/autokent/pdf-parse/-/merge_requests/3/diffs is still open; is that package still maintained?

@devsnek
Copy link
Member

devsnek commented Mar 11, 2021

aren't we in the middle of runtime deprecating module.parent?

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 11, 2021

aren't we in the middle of runtime deprecating module.parent?

Indeed: #33534

it also reinforces that this pattern shouldn't be used

Agreed, but in the mean time, the current behaviour hurts the wrong people: package users, rather than package authors.

@devsnek
Copy link
Member

devsnek commented Mar 11, 2021

So yeah, we should definitely not merge this.

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 13, 2023

It seems unnecessary to add this at this point, closing.

@aduh95 aduh95 closed this Apr 13, 2023
@aduh95 aduh95 deleted the truthy-module.parent branch April 13, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants