Skip to content
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

Jump to bottom

modules: add setter for module.parent #35522

Merged
merged 1 commit into from Oct 16, 2020
Merged

modules: add setter for module.parent #35522

merged 1 commit into from Oct 16, 2020

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 6, 2020

module.parent breaks the ecosystem when read-only (see #32217 (comment)). This PR adds a setter to allow user to change its value instead of throwing.

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@richardlau
Copy link
Member

CITGM: https://ci.nodejs.org/job/citgm-smoker/2498/

bmeck
bmeck approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending lint

richardlau
richardlau approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the CITGM results that have completed this has fixed the node-gyp test suite, thanks.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020
@MylesBorins

This comment has been minimized.

Sign in to view
@MylesBorins MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 6, 2020
@richardlau
Copy link
Member

Can we fast track? Would be good to get this in the next 14.x release

14.x isn't affected. #33533 didn't backport the breaking parts of #32217.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33430/

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 6, 2020 •
edited

BTW, this should have the label dont-land-on-v14.x and dont-land-on-v12.x.

@MylesBorins MylesBorins removed the fast-track PRs that do not need to wait for 48 hours to land. label Oct 6, 2020
@MylesBorins

This comment has been minimized.

Sign in to view
@ExE-Boss
Copy link
Contributor

ExE-Boss commented Oct 7, 2020 •
edited

My only issue with converting module.parent into an accessor on the prototype is that delete module.parent no longer works (unless module is converted into a Proxy) and module.parent = undefined triggers the deprecation warning.

bmeck
bmeck reviewed Oct 8, 2020
View reviewed changes
lib/internal/modules/cjs/loader.js
'module.parent is deprecated due to accuracy issues. Please use ' +
'require.main to find program entry point instead.',
'DEP0144'
) : setModuleParent,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make delete work:

Suggested change
) : setModuleParent,
) : setModuleParent,
configurable: true,

@bmeck
Copy link
Member

bmeck commented Oct 8, 2020

@ExE-Boss making the property configurable should make delete work.

bmeck
bmeck reviewed Oct 8, 2020
View reviewed changes
test/parallel/test-module-parent-setter-deprecation.js
'DEP0144'
);

module.parent = undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
module.parent = undefined;
module.parent = undefined;
delete module.parent;
assert.strictEqual('parent' in module, false);

bmeck
bmeck reviewed Oct 8, 2020
View reviewed changes
test/parallel/test-module-parent-setter-deprecation.js
// Flags: --pending-deprecation

'use strict';
const common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const common = require('../common');
const common = require('../common');
const assert = require('assert');

bmeck
bmeck reviewed Oct 8, 2020
View reviewed changes
lib/internal/modules/cjs/loader.js
function setModuleParent(value) {
moduleParentCache.set(this, value);
}

ObjectDefineProperty(Module.prototype, 'parent', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving this to be on the module itself is needed to make delete work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that trigger a new deprecation warning for each Module instance?

module.parent; // triggers one warning
module.parent?.parent; // triggers another warning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because the deprecation uses a deprecation code:

node/lib/internal/util.js

Lines 77 to 82 in ce84bac

if (code !== undefined) {
if (!codesWarned.has(code)) {
process.emitWarning(msg, 'DeprecationWarning', code, deprecated);
codesWarned.add(code);
}
} else {

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 13, 2020

@ExE-Boss Would it be fine for you if we land this PR as is, and then someone else (maybe you?) opens another PR with the changes you're suggesting? I'm afraid I don't have the bandwidth to work on this currently, and I wouldn't want it to miss the v15.x release.

@ExE-Boss
Copy link
Contributor

@aduh95 I don’t mind either way.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 14, 2020
@aduh95
module: add setter for module.parent
aaf225a 
PR-URL: nodejs#35522
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@aduh95
Copy link
Contributor Author

aduh95 commented Oct 16, 2020

Landed in aaf225a

@aduh95 aduh95 merged commit aaf225a into nodejs:master Oct 16, 2020
@aduh95 aduh95 deleted the module-parent-setter branch October 16, 2020 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmeck bmeck bmeck approved these changes

@ExE-Boss ExE-Boss ExE-Boss left review comments

@jkrems jkrems jkrems approved these changes

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@richardlau richardlau richardlau approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@aduh95 @nodejs-github-bot @richardlau @MylesBorins @ExE-Boss @bmeck @jkrems @addaleax @benjamingr @ryzokuken