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

modules: add setter for module.parent #35522

Merged
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

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

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.

@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

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 6, 2020

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.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Oct 7, 2020

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.

'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.

'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);

// 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');

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:

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
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
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants