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

doc: remove --experimental-modules documentation #38974

Closed
wants to merge 2 commits into from
Closed

doc: remove --experimental-modules documentation #38974

wants to merge 2 commits into from

Conversation

iam-frankqiu
Copy link
Contributor

Because ESM modules have been fully supported since 13.2.0.
So I think we should remove the experimental-modules option in case of misunderstanding.

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 9, 2021
@aduh95
Copy link
Contributor

aduh95 commented Jun 9, 2021

/cc @nodejs/modules

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 9, 2021
targos
targos requested changes Jun 9, 2021
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

I think this is way too early for this flag to be removed from node_options.cc.
Removing it only from the documentation SGTM, because it's a no-op and is not returned in node --help.

addaleax
addaleax reviewed Jun 11, 2021
View reviewed changes
src/node_options.cc Outdated Show resolved Hide resolved
@addaleax addaleax removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 21, 2021
@iam-frankqiu iam-frankqiu added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 21, 2021
jkrems
jkrems approved these changes Oct 21, 2021
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.

LGTM with the "make it a noop" suggestion from @addaleax.

MylesBorins
MylesBorins approved these changes Oct 22, 2021
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

+1 for undocumented noop

@aduh95 aduh95 changed the title refactor: remove experimental-modules flag doc: remove --experimental-modules documentation Oct 23, 2021
@aduh95
Copy link
Contributor

aduh95 commented Oct 23, 2021

One test is failing: test-process-env-allowed-flags-are-documented

@aduh95 aduh95 added doc Issues and PRs related to the documentations. and removed c++ Issues and PRs that require attention from people who are familiar with C++. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. labels Oct 23, 2021
@iam-frankqiu iam-frankqiu force-pushed the del_experimental_modules branch from 9c2e805 to 1ba1c12 Compare October 24, 2021 04:02
@iam-frankqiu iam-frankqiu force-pushed the del_experimental_modules branch from c655a71 to 7b26d20 Compare October 27, 2021 13:59
@iam-frankqiu iam-frankqiu requested a review from targos October 27, 2021 15:10
@iam-frankqiu iam-frankqiu added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 27, 2021
@targos targos self-assigned this Oct 31, 2021
targos pushed a commit that referenced this pull request Oct 31, 2021
@iam-frankqiu @targos
doc: remove --experimental-modules documentation
b63313a 
PR-URL: #38974
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
Copy link
Member

targos commented Oct 31, 2021

Landed in b63313a

@targos targos closed this Oct 31, 2021
@targos targos removed their assignment Oct 31, 2021
targos pushed a commit that referenced this pull request Nov 6, 2021
@iam-frankqiu @targos
doc: remove --experimental-modules documentation
c104f5a 
PR-URL: #38974
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Nov 8, 2021
Merged
BethGriggs pushed a commit that referenced this pull request Nov 25, 2021
@iam-frankqiu @BethGriggs
doc: remove --experimental-modules documentation
de5c5c8 
PR-URL: #38974
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

@jkrems jkrems jkrems approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@jasnell jasnell jasnell 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. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@iam-frankqiu @aduh95 @targos @jasnell @GeoffreyBooth @MylesBorins @jkrems @addaleax