modules: deprecate module.parent

[aduh95]

This feature does not work when a module is imported using ECMAScript modules
specification, therefore it is deprecated.

This has been discussed during Node.js Modules Team Meeting 2020-03-11; the main issue with this feature is that users are using it to check if the current module is the entry point, but that breaks when a CJS module is imported using ESM loader. Also its behaviour is kind of counterintuitive because of CJS cache.

This PR contains a breaking change: Object.keys(module).includes('parent') used to be true, and is now false. Hopefully this can get merged before v14 feature freeze.

Fixes: nodejs/modules#469

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[DerekNonGeneric]

❌I'm opposed

My dissent is voiced in a comment to the related issue.

nodejs/modules#469 (comment)

[devsnek]

even before esm existed this should've been deprecated

[bmeck]

While we are here, can we update https://github.com/nodejs/node/blob/master/doc/api/modules.md#moduleparent to state that the value may be undefined or null. Regardless of the deprecation logic, that is a driving force behind this PR.

[bmeck]

nits

[addaleax]

I’m -1 on a runtime deprecation just because a feature won’t always work as expected.

This only starts to break when people are moving code over to ESM. When it does break, it will most likely break in a way that’s easily noticed. I don’t think an extra runtime deprecation would be helpful.

[bmeck]

@addaleax do you feel comfortable with docs deprecation?

[aduh95]

@addaleax it breaks when people are using ESM or REPL, there is a better alternative that always works, and it seems there is no actual use-case for this feature.

Anyway, would you find it acceptable if the warning is displayed only when module.parent is undefined or null? This way the warning is displayed when the feature may break, hence helping the debug process.

[aduh95]

When it does break, it will most likely break in a way that’s easily noticed.

@addaleax I don't think that's a fair assumption: when I did encounter the issue, I was porting a library of mine to ESM, and it was not obvious what what did go wrong. One of the dependency is using a check on module.parent to run its tests, tests that have side effects but there were no console output indicating tests were being run. Even reading the dependency source code, it wasn't obvious what was causing the issue, I didn't know module.parent would be undefined because I was importing from ESM.

I know this is just anecdotal evidence, yet I am convinced a runtime warning is needed; hopefully my user story shows how Node.js can benefit from a runtime deprecation.

To avoid leaking warnings to library users, I have changed the algorithm to warn only when module.parent is null or undefined. My reasoning behind this choice is:

• Users that have a dependency checking module.parent won't get the warning when using require (because it works fine in that situation)
• Users that have a dependency checking module.parent will get the warning when using import (because it breaks in that situation)
• Library authors checking module.parent to run their tests will get the warning so they know they should fix their code.
• Users using a CLI package that checks module.parent will also get the warning, that's unfortunate but I would expect it is not a very common pattern.
• EDIT: I have added a condition to display the warning to users using --pending-deprecations flag regardless of the value of module.parent.

I have also added a code snippet in the deprecation note to tell users how to get the current module's parents to address @DerekNonGeneric's concern.

[addaleax]

I’ll remove my objection since the PR now warns in different situations, but I still think that this is not a good idea, tbh.

If the problem is that module.parent == null happens unexpectedly when importing from ESM, I would try to fix that by returning a fake object for module.parent in that situation – it doesn’t really have to be a full Module, just something as close to it as that makes sense.

I also still think that a full runtime deprecation is overkill, and I would feel more comfortable introducing this only for --pending-deprecation at first – especially as we generally aim to documentation-deprecate features before runtime-deprecating them.

outdated

[ExE-Boss]

Personally, I’d prefer if this emitted the warning for nullish parent values regardless of ‑‑pending‑deprecation being specified.

[bmeck]

@papb yes, we have talked about meow and similar for quite some time ( I see a ref from 2017 in nodejs/node-eps#57 (comment) )

[richardlau]

After bisecting, this PR is the reason that node-gyp's test suite is failing in CITGM on master and the current v15.0.0-proposal (#35014, cc @BethGriggs).
e.g. https://ci.nodejs.org/job/citgm-smoker/2497/nodes=ubuntu1804-64/testReport/junit/(root)/citgm/node_gyp_v7_1_0/

 # Subtest: test/test-configure-python.js
     1..0 # no tests found
 not ok 2 - test/test-configure-python.js # time=409.579ms
   ---
   timeout: 120000
   file: test/test-configure-python.js
   childId: 1
   command: /home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1804-64/smoker/bin/node
   args:
     - '-r'
     - >-
       /home/iojs/tmp/citgm_tmp/a30ded27-455a-421a-8399-2875b04856df/node-gyp/node_modules/esm/esm.js
     - test/test-configure-python.js
   stdio:
     - 0
     - pipe
     - 2
   cwd: /home/iojs/tmp/citgm_tmp/a30ded27-455a-421a-8399-2875b04856df/node-gyp
   exitCode: 1
   ...
...
 /home/iojs/tmp/citgm_tmp/a30ded27-455a-421a-8399-2875b04856df/node-gyp/node_modules/require-inject/index.js:72
   Object.keys(mocks).forEach(function (name) {
                      ^
 TypeError: Cannot set property parent of #<Module> which has only a getter
     at /home/iojs/tmp/citgm_tmp/a30ded27-455a-421a-8399-2875b04856df/node-gyp/node_modules/require-inject/index.js:82:18
     at Array.forEach (<anonymous>)
     at installGlobally (/home/iojs/tmp/citgm_tmp/a30ded27-455a-421a-8399-2875b04856df/node-gyp/node_modules/require-inject/index.js:72:22)
     at requireInject (/home/iojs/tmp/citgm_tmp/a30ded27-455a-421a-8399-2875b04856df/node-gyp/node_modules/require-inject/index.js:41:7)
     at module.exports (/home/iojs/tmp/citgm_tmp/a30ded27-455a-421a-8399-2875b04856df/node-gyp/node_modules/require-inject/index.js:13:10)
     at Object.<anonymous> (/home/iojs/tmp/citgm_tmp/a30ded27-455a-421a-8399-2875b04856df/node-gyp/test/test-configure-python.js:8:19)
     at Module._compile (internal/modules/cjs/loader.js:1072:30)
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1101:10)
     at Module.load (internal/modules/cjs/loader.js:937:32)
     at Function.Module._load (internal/modules/cjs/loader.js:778:14)

The test suite passes on Node.js 14.

@nodejs/modules-active-members is this expected to break for CJS? This was documented as being a documentation only deprecation.

[bmeck]

@richardlau this looks like whenever the deprecated form landed it didn't add back a setter, to my knowledge there was no intent to prevent assignment in CJS. This looks like a bug and we need to add back a setter, right now https://github.com/iarna/require-inject/blob/b1cc122fa34741a5d60ad4c26e839fd9bbb7a61b/index.js#L36 is failing due to the lack of setter. changing

node/lib/internal/modules/cjs/loader.js

Lines 226 to 233 in 8beef5e

	ObjectDefineProperty(Module.prototype, 'parent', {
	get: pendingDeprecation ? deprecate(
	getModuleParent,
	'module.parent is deprecated due to accuracy issues. Please use ' +
	'require.main to find program entry point instead.',
	'DEP0144'
	) : getModuleParent
	});

to have a setter should fix it

[aduh95]

This PR moved the parent declaration from the Module instance to the prototype to ensure the pending deprecation warning is written only once. As a result module.parent is now read-only. It's a breaking change, and has been backported to v14.x in semver-minor patch.

require-inject is apparently creating a Module instance, and tries to assign it a parent property which throws. A possible fix would be to add a setter to Module.prototype.parent.

EDIT: @bmeck beat me, sorry for the repetition.
I've opened iarna/require-inject#19 to try to solve the issue.

[guybedford]

FYI this is about to go out in 12.

[targos]

@guybedford The version of this change that's going out in 12 is #34592 (doc-only deprecation)

[guybedford]

Ah great, thanks for verifying!

…

On Tue, Oct 6, 2020 at 10:46 Michaël Zasso ***@***.***> wrote: @guybedford <https://github.com/guybedford> The version of this change that's going out in 12 is #34592 <#34592> (doc-only deprecation) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32217 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSVVXPE47WUCW7UCWGTSJNJXXANCNFSM4LGAHYGQ> .

[edumaciel10]

cool