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

errors: remove experimental from --enable-source-maps #37362

Closed
wants to merge 6 commits into from

Conversation

@bcoe
Copy link
Member

@bcoe bcoe commented Feb 13, 2021

--enable-source-maps has been a feature since v12.12.0 and is at a point where I would be comfortable calling the functionality stable.

Why I think this is a good call:

  • there are hundreds of folks setting the flag already on GitHub -- dropping the experimental should drive even more adoption.
  • I believe we've addressed most major outstanding bugs -- there's room for improvements, i.e., adding tests for worker threads, making them work in a REPL, but I think this work is additive.
  • #37252 addresses the concerns I had around our stack trace format not aligning with standards work that's happening.

CC: @ljharb

@bcoe
Copy link
Member Author

@bcoe bcoe commented Feb 13, 2021

@nodejs/tsc @nodejs/tooling for visibility.

@ljharb
Copy link
Member

@ljharb ljharb commented Feb 13, 2021

LGTM regarding concerns about the stacks proposal.

bcoe added 2 commits Feb 14, 2021
doc/api/cli.md Outdated Show resolved Hide resolved
@Trott
Trott approved these changes Feb 14, 2021
Copy link
Member

@Trott Trott left a comment

LGTM with minor tidying of bottom references. (Or even without that minor tidying--it can always be done in a subsequent PR, or just left as is.)

@iansu
iansu approved these changes Feb 14, 2021
@lpinca
lpinca approved these changes Feb 14, 2021
@aduh95
aduh95 approved these changes Feb 19, 2021
doc/api/cli.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Copy link
Member

@mhdawson mhdawson left a comment

LGTM

@nodejs-github-bot

This comment has been hidden.

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 19, 2021

@bcoe I've cancelled the Jenkins CI as it is not necessary for doc-only change.

For documentation-only changes, GitHub Actions CI is sufficient.

bcoe added a commit that referenced this pull request Feb 22, 2021
PR-URL: #37362
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ian Sutherland <ian@iansutherland.ca>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@bcoe
Copy link
Member Author

@bcoe bcoe commented Feb 22, 2021

Landed in 66ba0f1

@bcoe bcoe closed this Feb 22, 2021
cjihrig added a commit to cjihrig/node that referenced this pull request Feb 27, 2021
Refs: nodejs#37362
targos added a commit that referenced this pull request Feb 28, 2021
PR-URL: #37362
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ian Sutherland <ian@iansutherland.ca>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
cjihrig added a commit to cjihrig/node that referenced this pull request Mar 1, 2021
PR-URL: nodejs#37540
Refs: nodejs#37362
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
lpinca added a commit to lpinca/node that referenced this pull request Mar 2, 2021
PR-URL: nodejs#37540
Refs: nodejs#37362
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Mar 2, 2021
PR-URL: #37540
Refs: #37362
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Mar 2, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) make FIPS related options always awailable (Vít Ondruch) #36341
errors:
  * (SEMVER-MINOR) remove experimental from --enable-source-maps (Benjamin Coe) #37362

PR-URL: TODO
targos added a commit that referenced this pull request Mar 2, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) make FIPS related options always awailable (Vít Ondruch) #36341
errors:
  * (SEMVER-MINOR) remove experimental from --enable-source-maps (Benjamin Coe) #37362

PR-URL: #37569
targos added a commit that referenced this pull request Mar 3, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) make FIPS related options always awailable (Vít Ondruch) #36341
errors:
  * (SEMVER-MINOR) remove experimental from --enable-source-maps (Benjamin Coe) #37362

PR-URL: #37569
targos added a commit that referenced this pull request Mar 3, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) make FIPS related options always awailable (Vít Ondruch) #36341
errors:
  * (SEMVER-MINOR) remove experimental from --enable-source-maps (Benjamin Coe) #37362

PR-URL: #37569
@Flarna
Copy link
Member

@Flarna Flarna commented Mar 3, 2021

@bcoe Is it intended that node --help still shows experimental? See

node/src/node_options.cc

Lines 300 to 303 in f6b1df2

AddOption("--enable-source-maps",
"experimental Source Map V3 support",
&EnvironmentOptions::enable_source_maps,
kAllowedInEnvironment);

DerekNonGeneric added a commit to DerekNonGeneric/node-runtime that referenced this pull request Mar 4, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) make FIPS related options always awailable (Vít Ondruch) nodejs#36341
errors:
  * (SEMVER-MINOR) remove experimental from --enable-source-maps (Benjamin Coe) nodejs#37362

PR-URL: nodejs#37569
@bcoe
Copy link
Member Author

@bcoe bcoe commented Mar 4, 2021

@Flarna this was an oversight 👏 thanks for pointing it out.

I will submit a patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet