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

module: unflag Top-Level Await #34558

Closed
wants to merge 1 commit into from
Closed

Conversation

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Jul 29, 2020

This unflags Top-Level await so it can be used by default in the module
goal. This is accomplished by manually setting the
--harmony-top-level-await flag. We are allowing this as a one of
approval based on circumstances. It is not a precedent that future
harmony features will be manually enabled.

Refs: #34551

Some questions:

  • Is this how we want to manually set the harmony flag?
    • yes
  • Do we want to keep the original flag as a no-op as implemented here
    • yes
  • Do we want to output a warning when TLA is used?
    • no
  • Are we ok with landing this in advance of V8 unflagging this feature (PTAL at the above referenced issue for more details)
    • yes
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jul 29, 2020

src/node_options.cc Show resolved Hide resolved
@ljharb
Copy link
Member

@ljharb ljharb commented Jul 29, 2020

It seems like, if we're going to "unflag" TLA, we should not provide a way to turn it off - unless v8 is planning to provide one looking forward. Otherwise, updating to a version of v8 where TLA is enabled by default becomes a breaking change, since there'd no longer be any way to turn it off.

@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Jul 29, 2020

@ljharb if we continue to mark this feature as experimental then removing that functionality would not be Semver-Major. It seems like finding a way to limit that functionality would be a lot of extra work.

@ljharb
Copy link
Member

@ljharb ljharb commented Jul 29, 2020

I'm not familiar with this part of node, to be sure, but it seems strange that it'd be difficult to turn --flag or --no-flag into noops, and then still interject --flag into what's passed to v8.

doc/api/esm.md Show resolved Hide resolved
@MylesBorins MylesBorins force-pushed the unflag-tla branch 3 times, most recently from 4bc48bb to 2d6743a Jul 31, 2020
@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Jul 31, 2020

Hey All,

I think this is ready to go PTAL

@ljharb
Copy link
Member

@ljharb ljharb commented Jul 31, 2020

to close the loop from my above comments around the ability to disable TLA, since apparently v8 has a "staged" phase where a feature is enabled by default but a flag remains to disable it, this PR matches that, so, LGTM (altho I think it's weird to ever have a flag that lets you turn off a language feature)

@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Jul 31, 2020

Copy link
Member

@devsnek devsnek left a comment

marking requested changes until no-tla test is added

Copy link
Member

@mcollina mcollina left a comment

lgtm

@nodejs-github-bot

This comment has been hidden.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jul 31, 2020

devsnek
devsnek approved these changes Aug 1, 2020
@nodejs-github-bot

This comment has been hidden.

@addaleax
Copy link
Member

@addaleax addaleax commented Aug 6, 2020

Just so the context is here as well, I’ve added dont-land-on-v14.x because I think that this should not be released without #34640, and dont-land-on-v12.x because my understanding is that it wouldn’t apply there.

@codebytere
Copy link
Member

@codebytere codebytere commented Aug 6, 2020

@addaleax should this perhaps be backport-blocked for 14, since there is a path forward?

@addaleax
Copy link
Member

@addaleax addaleax commented Aug 6, 2020

@codebytere I don’t know – is there a practical difference? The next step would be to just remove the label again once #34640 lands, regardless of which one is used, right? Anyway, feel free to put the correct one here if I got it wrong. :)

addaleax added a commit that referenced this issue Aug 8, 2020
Handle situations in which the main `Promise` from a TLA module
is not fulfilled better:

- When not resolving the `Promise` at all, set a non-zero exit code
  (unless another one has been requested explicitly) to distinguish
  the result from a successful completion.
- When rejecting the `Promise`, always treat it like an uncaught
  exception. In particular, this also ensures a non-zero exit code.

Refs: #34558

PR-URL: #34640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
addaleax added a commit that referenced this issue Aug 8, 2020
This unflags Top-Level await so it can be used by default in the module
goal. This is accomplished by manually setting the
--harmony-top-level-await flag. We are allowing this as a one of
approval based on circumstances. It is not a precedent that future
harmony features will be manually enabled.

Refs: #34551

PR-URL: #34558
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
addaleax added a commit that referenced this issue Aug 8, 2020
Handle situations in which the main `Promise` from a TLA module
is not fulfilled better:

- When not resolving the `Promise` at all, set a non-zero exit code
  (unless another one has been requested explicitly) to distinguish
  the result from a successful completion.
- When rejecting the `Promise`, always treat it like an uncaught
  exception. In particular, this also ensures a non-zero exit code.

Refs: #34558

PR-URL: #34640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@codebytere codebytere mentioned this pull request Aug 10, 2020
codebytere added a commit that referenced this issue Aug 10, 2020
Notable changes:

async_hooks:
  * (SEMVER-MINOR) add AsyncResource.bind utility (James M Snell) #34574
doc:
  * add Ricky Zhou to collaborators (rickyes) #34676
  * add release key for Ruy Adorno (Ruy Adorno) #34628
  * add DerekNonGeneric to collaborators (Derek Lewis) #34602
module:
  * (SEMVER-MINOR) unflag Top-Level Await (Myles Borins) #34558
n-api:
  * (SEMVER-MINOR) support type-tagging objects (Gabriel Schulhof) #28237
n-api,src:
  * (SEMVER-MINOR) provide asynchronous cleanup hooks (Anna Henningsen) #34572

PR-URL: #34704
codebytere added a commit that referenced this issue Aug 11, 2020
This unflags Top-Level await so it can be used by default in the module
goal. This is accomplished by manually setting the
--harmony-top-level-await flag. We are allowing this as a one of
approval based on circumstances. It is not a precedent that future
harmony features will be manually enabled.

Refs: #34551

PR-URL: #34558
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
codebytere added a commit that referenced this issue Aug 11, 2020
Handle situations in which the main `Promise` from a TLA module
is not fulfilled better:

- When not resolving the `Promise` at all, set a non-zero exit code
  (unless another one has been requested explicitly) to distinguish
  the result from a successful completion.
- When rejecting the `Promise`, always treat it like an uncaught
  exception. In particular, this also ensures a non-zero exit code.

Refs: #34558

PR-URL: #34640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
codebytere added a commit that referenced this issue Aug 11, 2020
Notable changes:

async_hooks:
  * (SEMVER-MINOR) add AsyncResource.bind utility (James M Snell) #34574
doc:
  * add Ricky Zhou to collaborators (rickyes) #34676
  * add release key for Ruy Adorno (Ruy Adorno) #34628
  * add DerekNonGeneric to collaborators (Derek Lewis) #34602
module:
  * (SEMVER-MINOR) unflag Top-Level Await (Myles Borins) #34558
n-api:
  * (SEMVER-MINOR) support type-tagging objects (Gabriel Schulhof) #28237
n-api,src:
  * (SEMVER-MINOR) provide asynchronous cleanup hooks (Anna Henningsen) #34572

PR-URL: #34704
codebytere added a commit that referenced this issue Aug 11, 2020
Notable changes:

async_hooks:
  * (SEMVER-MINOR) add AsyncResource.bind utility (James M Snell) #34574
doc:
  * add Ricky Zhou to collaborators (rickyes) #34676
  * add release key for Ruy Adorno (Ruy Adorno) #34628
  * add DerekNonGeneric to collaborators (Derek Lewis) #34602
module:
  * (SEMVER-MINOR) unflag Top-Level Await (Myles Borins) #34558
n-api:
  * (SEMVER-MINOR) support type-tagging objects (Gabriel Schulhof) #28237
n-api,src:
  * (SEMVER-MINOR) provide asynchronous cleanup hooks (Anna Henningsen) #34572

PR-URL: #34704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment