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

Unflag Top-Level Await? #34551

Closed
MylesBorins opened this issue Jul 29, 2020 · 17 comments
Closed

Unflag Top-Level Await? #34551

MylesBorins opened this issue Jul 29, 2020 · 17 comments

Comments

@MylesBorins
Copy link
Contributor

We have top-level await flagged on v14.x / master behind --experimental-top-level-await.

I think we should consider unflagging, but keeping a warning when it is used the first time.

Thoughts?

@richardlau
Copy link
Member

For clarification, this is unflagging on 14.x or the next semver major?

@devsnek
Copy link
Member

devsnek commented Jul 29, 2020

I don't think we should unflag until it is stable in V8. I think as a policy we should never enable harmony flags by default.

@jasnell
Copy link
Member

jasnell commented Jul 29, 2020

Have to agree with @devsnek ... given the dependency on V8, I think we should hold off.

@MylesBorins
Copy link
Contributor Author

@jasnell I can confirm with the V8 team but my understanding is that the implementation in V8 is stable. Currently what is blocking the stability upstream is the HTML integration

@MylesBorins
Copy link
Contributor Author

@richardlau I was suggesting to unflag for 14.x. As TLA was specified it's enablement should have 0 observable changes if it is not used.

@devsnek
Copy link
Member

devsnek commented Jul 29, 2020

@MylesBorins to be clear, I realize the tla implementation itself is stable. the problem is that it is flagged as an unstable feature, and I don't want precedence of messing around with that.

@MylesBorins
Copy link
Contributor Author

@devsnek while I can understand that, we can be explicit that this is a one-off, not a precedent. This feature is a huge boon to the ecosystem and if what we are shipping is highly unlikely to change I don't think we are benefiting from keeping it flagged.

@guybedford
Copy link
Contributor

I'd be strongly in support of this unflagging as a specific one-off exception for v8 unflagging. Just yesterday I was using this flag to great benefit myself - it's such a common scenario and help to Node.js users I think it would be a massive win for our users that is long overdue already.

@syg
Copy link
Contributor

syg commented Jul 29, 2020

The V8-side implementation of TLA has been staged since May, so it's been getting clusterfuzz coverage for 2-3 months. From that perspective we can be more confident that the V8 part of this is more stable (in terms of bugs) than not. Note that it is still not as stable as on-by-default.

The current hold up to ship is HTML integration, namely, working out timing issues around firing events like onload and onerror for module scripts, as TLA moves work to the microtask queue.

IOW, the thing preventing us from shipping and being iterated on isn't in V8.

@bnb
Copy link
Contributor

bnb commented Jul 29, 2020

I'd be strongly in support of this unflagging as a specific one-off exception for v8 unflagging. Just yesterday I was using this flag to great benefit myself - it's such a common scenario and help to Node.js users I think it would be a massive win for our users that is long overdue already.

Expanding on this, when first starting to use async/await TLA was one of the big gotchas I ran into. It is a genuinely good piece of DX and as the ecosystem starts moving forward it would be particularly positive, especially as we're moving into a point where a large amount of tooling and content is about to be generated about using modern features of the project, I think it would likely better for the ecosystem's advancement if we can help enable people to use this sooner rather than later - especially if it is relatively stable (despite being unmarked as such) in V8.

Given the unflagged experimental support for ESM and the scoping of TLA to the module target (AFAIK?), I don't think it's a particular stretch to enable it.

@robpalme
Copy link

I agree with @bnb and @guybedford that now is the right time to unflag - specifically whilst we have a small user group on ESM.

It ensures that any Node-specific integration risk for TLA affects only brave users. And it also acts as a carrot for would-be ESM users to try ESM out before Node declares ESM non-experimental and widespread adoptions kicks in.

@mmarchini
Copy link
Contributor

I'm also +1, the benefits for our users outweigh the risk of this becoming a precedent (and as @MylesBorins said, we can explicitly say this is not a precedent but instead a one-off).

@jasnell
Copy link
Member

jasnell commented Jul 29, 2020

I certainly won't block given that it appears to be fairly stable in V8, but I definitely don't want to set any kind of precedence with regards to shipping with harmony flags enabled. This would definitely need to be an exception.

@devsnek
Copy link
Member

devsnek commented Jul 29, 2020

it might be good to establish 2-3 people (I can be one) who can quickly upstream patches to V8, given we're sort of taking on any potentially broken behaviour here as our own.

@mmarchini
Copy link
Contributor

I can open PRs upstream if necessary (I don't have commit access there though)

@MylesBorins
Copy link
Contributor Author

I've helped get things landed on V8 before. I can't volunteer to necessarily help with fixing the problems but I can utilize my experience and relationships to get things landed.

@MylesBorins
Copy link
Contributor Author

Closing as discussion seemed to be positive about landing and there is an open PR

MylesBorins added a commit to MylesBorins/node that referenced this issue Aug 3, 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: nodejs#34551
MylesBorins added a commit that referenced this issue Aug 3, 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 pushed 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>
codebytere pushed 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants