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.

Sign up for GitHub

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

[v14.x backport] modules: deprecate module.parent #33533

Closed
wants to merge 5 commits into from
Closed

[v14.x backport] modules: deprecate module.parent #33533

wants to merge 5 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 24, 2020 •
edited
Loading

Backport of #32217

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

@aduh95 aduh95 force-pushed the backport-32217-to-current branch from 8795a25 to 51e1517 Compare May 24, 2020 09:04
@BethGriggs
Copy link
Member

ping @nodejs/modules-active-members, would appreciate some reviews before landing this in v14.x

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 9, 2020

Let me rebase to make the diff more readable!

aduh95 added 2 commits June 9, 2020 17:25
@aduh95
module: deprecate module.parent
281c4f7 
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 <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@aduh95
Remove prending deprecation for backporting
c88cca4
@aduh95 aduh95 force-pushed the backport-32217-to-current branch from c51060d to c88cca4 Compare June 9, 2020 15:52
@ljharb
Copy link
Member

ljharb commented Jun 9, 2020

LGTM

@aduh95
Copy link
Contributor Author

aduh95 commented Jul 1, 2020

This didn't make it to 14.5.0, is there something I can do to help it land in the next semver-minor?

@MylesBorins MylesBorins self-assigned this Jul 2, 2020
@MylesBorins
Copy link
Contributor

I'm about to head on vacation but can help bring this over the finish line after I get back in a week

@MylesBorins
Copy link
Contributor

@nodejs/lts @nodejs/tsc

There was a conflict with this file because another deprecation landed first and the deprecation number this PR was planning to use (and what it uses on master). It dawned on me that we might have inconsistent dep numbers across release streams. Is this intentional? Is this what we want to see happen? Happy to just move forward but wanted to flag.

@nodejs-github-bot
Copy link
Collaborator

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

@MylesBorins
Copy link
Contributor

@nodejs/lts do we have a policy around landing deprecations like this? Should it come in a semver-minor/

jkrems
jkrems approved these changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

Looks good in general but I share Myles' confusion about how deprecation numbering is expected to work here. But that may be a larger issue that this backport.

@targos
Copy link
Member

targos commented Jul 14, 2020

I'm generally in favor of backporting documentation-only deprecations as far back as possible.
I think deprecations are semver-minor by policy, but maybe I don't remember correctly.

@targos
Copy link
Member

targos commented Jul 14, 2020

About numbering, my opinion is that we should have consistent numbers across all release lines (equal to the numbers assigned when the commit lands on the master branch). I think it's OK to skip deprecation numbers if something isn't backported.

@jkrems
Copy link
Contributor

jkrems commented Jul 14, 2020

Did we already release conflicting deprecation numbers? If so, does it make sense to do a renumbering PR to each release line, potentially even dropping the numbers that may be ambiguous now (and having a footnote that they may refer to different deprecations, depending on where they have been seen)?

@richardlau
Copy link
Member

I'm kind of surprised the deprecation numbers between 14.x and master got out of sync. Did we land a deprecation in 14.x that didn't land in master first?

@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 14, 2020
@MylesBorins
Copy link
Contributor

Adding the semver-minor label as per the original PR

@BethGriggs
Copy link
Member

I'm kind of surprised the deprecation numbers between 14.x and master got out of sync. Did we land a deprecation in 14.x that didn't land in master first?

I believe so - #33126 (which was a slightly special case).

+1 to skipping numbers for consistency across release lines. I'm also leaning towards @jkrems suggestion of a PR to patch up the recent numbers.

I think it would make sense for Transform._transformState(#33126) to keep DEP0143, and deprecate module.parent to get bumped to the next available number as deprecate module.parent (#32217) has not gone out in a formal release yet (excluding nightlies).

@BethGriggs BethGriggs mentioned this pull request Jul 14, 2020
Closed
2 tasks
@MylesBorins
Copy link
Contributor

So after looking into it a bit more it seems like the deprecation number issue we are running into is because the original PR assigned a number instead of waiting for the release to assign the number, this is why we have two number assigned.

@richardlau
Copy link
Member

richardlau commented Jul 14, 2020 •
edited
Loading

Deprecation numbers are supposed to be assigned when the PR lands: https://github.com/nodejs/node/blob/master/doc/guides/releases.md#step-3-update-any-replaceme-and-dep00xx-tags-in-the-docs

@MylesBorins
Copy link
Contributor

@richardlau that step in the release guide is saying to replace DEP00XX with a deprecation code during the release. The code is not supposed to be assigned until that point

@richardlau
Copy link
Member

@MylesBorins it says

The code must be assigned a number (e.g. DEP0012). This assignment should occur when the PR is landed, but a check will be made when the release build is run.

We have definitely been assigning the codes directly on the master branch and not at release time, e.g.

@MylesBorins
Copy link
Contributor

I guess I'm just not up to date on the process changes that have happened. Not a huge deal.

Seems like not assigning until release and then updating on master is a good way to ensure codes stay in sync, but understand why we may not want to do that.

Don't really have strong feelings here.

@github-actions github-actions bot mentioned this pull request Jul 15, 2020
Open
13 tasks
ExE-Boss
ExE-Boss reviewed Jul 15, 2020
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Contributor

landed in 8795ee5

MylesBorins pushed a commit that referenced this pull request Jul 15, 2020
@aduh95 @MylesBorins
module: deprecate module.parent
8795ee5 
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 <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@github-actions github-actions bot mentioned this pull request Jul 16, 2020
Open
13 tasks
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
@aduh95 @MylesBorins
module: deprecate module.parent
8d7330b 
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 <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This was referenced Jul 17, 2020
Open
Open
Open
Open
Open
@aduh95 aduh95 deleted the backport-32217-to-current branch August 1, 2020 14:47
@aduh95 aduh95 mentioned this pull request Aug 1, 2020
Closed
3 tasks
@richardlau richardlau mentioned this pull request Oct 6, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@MylesBorins MylesBorins MylesBorins left review comments

@jkrems jkrems jkrems approved these changes

Assignees

@MylesBorins MylesBorins

Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@aduh95 @BethGriggs @ljharb @MylesBorins @nodejs-github-bot @targos @jkrems @richardlau @ExE-Boss