GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — repl: unflag Top-Level Await by hemanth · Pull Request #34733 · nodejs/node

Conversation

@hemanth

@hemanth hemanth commented Aug 11, 2020

Copy link
Copy Markdown
Contributor

Now that we have #34558 it makes sense to unflag Top-Level await for the REPL?

This PR is getting rid of --experimental-repl-await flag and the checks related to the same.

❯ node
Welcome to Node.js v14.8.0.
Type ".help" for more information.
> process.version
'v14.8.0'
> await Promise.resolve(123)
await Promise.resolve(42)
^^^^^

Uncaught SyntaxError: await is only valid in async function
❯ ./node
Welcome to Node.js v15.0.0-pre.
Type ".help" for more information.
> await Promise.resolve(42)
42

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95, tk120404, bricss, gengjiawen, bl-ue, JacksonTian, LinusU, and aomarks reacted with thumbs up emoji
All reactions
  • 👍 8 reactions
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. repl Issues and PRs related to the REPL subsystem. labels Aug 11, 2020
devsnek
devsnek previously requested changes Aug 11, 2020
View reviewed changes

@devsnek devsnek left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is not the same thing and is not as stable or useful. I don't think we should unflag this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@BridgeAR

BridgeAR commented Aug 11, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I agree with @devsnek

The implementation is very different from the actual top level await. There's very likely quite some work necessary to get the actual top level await working in the REPL.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@devsnek

devsnek commented Aug 11, 2020

Copy link
Copy Markdown
Member

We should be able to get this for free once v8:10539 is fixed

BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hemanth

hemanth commented Aug 11, 2020

Copy link
Copy Markdown
Contributor Author

@devsnek Agree it is not related, but this is the closest we can get to it?
Also, why was it introduced behind a flag?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BridgeAR

BridgeAR commented Aug 11, 2020

Copy link
Copy Markdown
Member

@hemanth it uses acorn to parse the code to wrap it in an async IIFE. That changes the way how let and const work and it has some other minor differences. That's why it's behind a flag.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell reviewed Aug 11, 2020
View reviewed changes
Comment thread src/node_options.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

@hemanth

hemanth commented Aug 11, 2020

Copy link
Copy Markdown
Contributor Author

Yes noticed @BridgeAR noticed that in internal/repl/await.js my intent was that we have TLA emulation in devtools and it would be useful to have the same in the REPL?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hemanth
hemanth force-pushed the unflag-tla-repl branch from 227ef06 to f257883 Compare August 11, 2020 20:29
@hemanth

hemanth commented Aug 11, 2020

Copy link
Copy Markdown
Contributor Author

Fixed the failing test with: assert(undocumented.delete('--experimental-repl-await')); but noticing the below:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

'SIGILL' !== null

    at Object.<anonymous> (/node/test/parallel/test-macos-app-sandbox.js:64:10)

How are these two related?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hemanth
hemanth force-pushed the unflag-tla-repl branch from f257883 to 03225c5 Compare August 11, 2020 22:42
@hemanth

hemanth commented Aug 12, 2020

Copy link
Copy Markdown
Contributor Author

Everything is green, except test-asan/test-asan says there is a memory leak.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@guybedford
guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33
@Ayase-252 Ayase-252 mentioned this pull request Jun 5, 2021
[Feature Request]: Add promise resolving in REPL #38921
Closed
@aduh95 aduh95 mentioned this pull request Jun 24, 2021
repl: unflag --experimental-repl-await #39142
Closed
@guybedford

guybedford commented Jul 2, 2021

Copy link
Copy Markdown
Contributor

@hemanth thanks for posting this.... I'm not trying to take over your work, but recently posted #39142 before I saw your PR. Would you be ok with closing this PR to continue to track that as the main PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hemanth

hemanth commented Jul 6, 2021

Copy link
Copy Markdown
Contributor Author

Oh nice @guybedford was just trying to understand the difference between these two PRs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@guybedford

guybedford commented Jul 7, 2021

Copy link
Copy Markdown
Contributor

@hemanth only differences are the rebase and not having the current block by @devsnek, but I think that applies to both regardless. If you are able to update the rebase here will gladly close the duplicate.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hemanth

hemanth commented Jul 7, 2021

Copy link
Copy Markdown
Contributor Author

Thank you! @guybedford I have resolved the conflicts.

I shall wait for @devsnek to unblock 🤓

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@devsnek
devsnek dismissed their stale review July 7, 2021 15:15

​

@hemanth
hemanth force-pushed the unflag-tla-repl branch from 339ab58 to c38bf81 Compare July 7, 2021 18:07
@nodejs-github-bot

This comment has been minimized.

Sign in to view
guybedford
guybedford approved these changes Jul 8, 2021
View reviewed changes

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It looks like Gus has removed his block here, so we might be good to go on this unless anyone else objects? I would personally wait on #39290 before landing this unflagging. Further implementation feedback very welcome //cc @ejose19.

@hemanth the CI can't run at the moment due to the rebase not landing cleanly - can you try squash this down to a single commit without a rebase commit? Then I can retrigger the CI.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@hemanth

hemanth commented Jul 8, 2021

Copy link
Copy Markdown
Contributor Author

image

Thanks @guybedford!

But looks like there is a mismatch from the git history on my CLI vs github.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@guybedford

guybedford commented Jul 8, 2021

Copy link
Copy Markdown
Contributor

@hemanth that top commit in your screenshot looks like a merge commit which is likely the issue.

hemanth reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hemanth
repl: unflag top-level await
a9d6eb4
unflags top-level await for the REPL.
This is accomplished by getting rid of `--experimental-repl-await` flag and the checks related to the same.
@hemanth
hemanth force-pushed the unflag-tla-repl branch from 503c640 to a9d6eb4 Compare July 8, 2021 18:37
@hemanth

hemanth commented Jul 8, 2021

Copy link
Copy Markdown
Contributor Author

Dang, right. Thanks @guybedford the latest commit should fix it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@hemanth
fixup: alphabetic order of flags.
a9f1e00
@hemanth

hemanth commented Jul 15, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Looks like finally everything is green! [Should I re-write the commit message?]

Also, made a quick video:

await-repl.mov

^ Look like GH is not able to play it inline, here is the link.

LinusU reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Jul 15, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Jul 16, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

guybedford pushed a commit that referenced this pull request Jul 16, 2021
@hemanth @guybedford
repl: enable --experimental-repl-await /w opt-out
14b26e0
Unflags top-level await for the REPL by enabling
--experimental-repl-await by default. Opt-out is
supported via --no-experimental-repl-await.

PR-URL: #34733
Reviewed-By: Guy Bedford <[email protected]>
@guybedford

guybedford commented Jul 16, 2021

Copy link
Copy Markdown
Contributor

Landed in 14b26e0. Thank you @hemanth for pushing this on through!

ejose19 and LinusU reacted with thumbs up emoji hemanth and LinusU reacted with heart emoji
All reactions
  • 👍 2 reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@guybedford guybedford closed this Jul 16, 2021
@hemanth

hemanth commented Jul 16, 2021

Copy link
Copy Markdown
Contributor Author

Thank you! @guybedford 👏🤸‍♂️

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cspotcode

cspotcode commented Jul 16, 2021

Copy link
Copy Markdown

Is this considered a breaking change or no? I'm wondering in which node version(s) it will release.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@guybedford guybedford added dont-land-on-v12.x labels Jul 16, 2021
@guybedford

guybedford commented Jul 16, 2021

Copy link
Copy Markdown
Contributor

@cspotcode strictly speaking this isn't a breaking change, so it would be fine for 16, but for stability reasons it probably is best not to backport. I've added the no backport labels.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cspotcode

cspotcode commented Jul 16, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

I see, you are saying that it will land in the next minor release of node 16 but not in any version of 14 nor 12, correct?

Where are the labels documented? It is often useful to know exactly which node versions will include a particular pull request, and of course I would like to avoid asking people if I can figure out this information on my own.

I briefly scanned these two documents and did not find any info about labels nor the process by which releases are prepared:
https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md
https://github.com/nodejs/node/blob/master/CONTRIBUTING.md

EDIT I think I found the right docs:
https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md
https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md
https://github.com/nodejs/node/blob/master/doc/guides/releases.md

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@guybedford guybedford added notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. labels Jul 16, 2021
@guybedford

guybedford commented Jul 16, 2021

Copy link
Copy Markdown
Contributor

@cspotcode yes exactly, glad you found the resoures there. I've just added the semver-minor and notable-change labels as well here. Let me know if you have any concerns or feedback here further.

cspotcode reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cspotcode

cspotcode commented Jul 16, 2021

Copy link
Copy Markdown

No feedback, this answers all my questions. Thank you.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos pushed a commit that referenced this pull request Jul 17, 2021
@hemanth @targos
repl: enable --experimental-repl-await /w opt-out
b67214f
Unflags top-level await for the REPL by enabling
--experimental-repl-await by default. Opt-out is
supported via --no-experimental-repl-await.

PR-URL: #34733
Reviewed-By: Guy Bedford <[email protected]>
BethGriggs added a commit that referenced this pull request Jul 26, 2021
@BethGriggs
2021-07-27, Version 16.6.0 (Current)
45fe4ab
Notable changes:

build:
  * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #39470
deps:
  * (SEMVER-MINOR) restore minimum ICU version to 68 (Michaël Zasso) #39470
  * (SEMVER-MINOR) make V8 9.2 abi-compatible with 9.0 (Michaël Zasso) #39470
  * (SEMVER-MINOR) update V8 to 9.2.230.21 (Michaël Zasso) #39470
inspector:
  * mark as stable (Gireesh Punathil) #37748
perf_hooks:
  * (SEMVER-MINOR) web performance timeline compliance (legendecas) #39297
punycode:
  * add pending deprecation (Antoine du Hamel) #38444
repl:
  * (SEMVER-MINOR) enable --experimental-repl-await /w opt-out (hemanth.hm) #34733

PR-URL: TODO
@BethGriggs BethGriggs mentioned this pull request Jul 26, 2021
v16.6.0 proposal #39534
Merged
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
@hemanth @BethGriggs
repl: enable --experimental-repl-await /w opt-out
a082a70
Unflags top-level await for the REPL by enabling
--experimental-repl-await by default. Opt-out is
supported via --no-experimental-repl-await.

PR-URL: #34733
Reviewed-By: Guy Bedford <[email protected]>
BethGriggs added a commit that referenced this pull request Jul 29, 2021
@BethGriggs
2021-07-29, Version 16.6.0 (Current)
21f12e1
This is a security release.

Notable Changes:

- CVE-2021-22930: Use after free on close http2 on stream canceling
  (High) [#39423](#39423)
- (SEMVER-MINOR) deps: update V8 to 9.2.230.21 (Michaël Zasso)
  [#39470](#39470)
- inspector: mark as stable (Gireesh Punathil)
  [#37748](#37748)
- (SEMVER-MINOR) perf_hooks: web performance timeline compliance
  (legendecas) [#39297](#39297)
- punycode: add pending deprecation (Antoine du Hamel)
  [#38444](#38444)
- (SEMVER-MINOR) repl: enable --experimental-repl-await /w opt-out
  (hemanth.hm) [#34733](#34733)

PR-URL: #39534
BethGriggs added a commit that referenced this pull request Jul 29, 2021
@BethGriggs
2021-07-29, Version 16.6.0 (Current)
8d2e66c
This is a security release.

Notable Changes:

- CVE-2021-22930: Use after free on close http2 on stream canceling
  (High) [#39423](#39423)
- (SEMVER-MINOR) deps: update V8 to 9.2.230.21 (Michaël Zasso)
  [#39470](#39470)
- inspector: mark as stable (Gireesh Punathil)
  [#37748](#37748)
- punycode: add pending deprecation (Antoine du Hamel)
  [#38444](#38444)
- (SEMVER-MINOR) repl: enable --experimental-repl-await /w opt-out
  (hemanth.hm) [#34733](#34733)

PR-URL: #39534
BethGriggs added a commit that referenced this pull request Jul 29, 2021
@BethGriggs
2021-07-29, Version 16.6.0 (Current)
0e22fa0
This is a security release.

Notable Changes:

- CVE-2021-22930: Use after free on close http2 on stream canceling
  (High) [#39423](#39423)
- (SEMVER-MINOR) deps: update V8 to 9.2.230.21 (Michaël Zasso)
  [#39470](#39470)
- inspector: mark as stable (Gireesh Punathil)
  [#37748](#37748)
- punycode: add pending deprecation (Antoine du Hamel)
  [#38444](#38444)
- (SEMVER-MINOR) repl: enable --experimental-repl-await /w opt-out
  (hemanth.hm) [#34733](#34733)

PR-URL: #39534
@aduh95

aduh95 commented Aug 12, 2021

Copy link
Copy Markdown
Contributor

We're getting issues with this implementation: #39744. Should we revert? It doesn't seem to be ready to be out of experimental just yet.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Aug 12, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Given that #39744 only happens if top-level await is used, I think it's fine.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Aug 12, 2021

Copy link
Copy Markdown
Member

I only considered this PR as unflagging, but not as moving out of experimental. If you think it would be good to add an explicit experimental status to the support of await in the docs, it SGTM.

aduh95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@guybedford

guybedford commented Aug 12, 2021

Copy link
Copy Markdown
Contributor

Thanks for the pointer. Bugs on experimental features are from a process perspective a feature not a bug!

aduh95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coderabbitai coderabbitai Bot mentioned this pull request Sep 12, 2026
Prepare Geddes 1.0: XRD readers, x/y API, and package checks jcwang587/geddes#57
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@guybedford guybedford guybedford approved these changes
@richardlau richardlau richardlau left review comments
+2 more reviewers
@targos targos targos left review comments
@devsnek devsnek devsnek left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. notable-change PRs with changes that should be highlighted in changelogs. repl Issues and PRs related to the REPL subsystem. 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.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@hemanth @BridgeAR @devsnek @guybedford @nodejs-github-bot @targos @cspotcode @aduh95 @jasnell @richardlau
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.