repl: unflag Top-Level Await #34733

hemanth · 2020-08-11T19:30:42Z

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

devsnek

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

BridgeAR · 2020-08-11T19:51:55Z

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.

devsnek · 2020-08-11T19:53:16Z

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

hemanth · 2020-08-11T19:57:19Z

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

BridgeAR · 2020-08-11T20:02:23Z

@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.

src/node_options.cc

hemanth · 2020-08-11T20:12:37Z

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?

hemanth · 2020-08-11T22:40:48Z

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?

hemanth · 2020-08-12T02:32:38Z

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

guybedford · 2021-07-02T22:58:40Z

@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?

hemanth · 2021-07-06T17:28:59Z

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

guybedford · 2021-07-07T13:17:03Z

@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.

hemanth · 2021-07-07T14:51:55Z

Thank you! @guybedford I have resolved the conflicts.

I shall wait for @devsnek to unblock 🤓

guybedford

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.

hemanth · 2021-07-08T18:16:06Z

Thanks @guybedford!

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

guybedford · 2021-07-08T18:22:01Z

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

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 · 2021-07-08T18:38:26Z

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

hemanth · 2021-07-15T04:22:52Z

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.

nodejs-github-bot · 2021-07-15T23:19:28Z

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

nodejs-github-bot · 2021-07-16T00:34:14Z

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

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 <guybedford@gmail.com>

guybedford · 2021-07-16T22:15:17Z

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

hemanth · 2021-07-16T22:24:02Z

Thank you! @guybedford 👏🤸‍♂️

cspotcode · 2021-07-16T22:50:42Z

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

guybedford · 2021-07-16T22:58:19Z

@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.

cspotcode · 2021-07-16T23:05:48Z

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

guybedford · 2021-07-16T23:19:25Z

@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 · 2021-07-16T23:22:42Z

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

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 <guybedford@gmail.com>

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

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 <guybedford@gmail.com>

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

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 · 2021-08-12T08:27:12Z

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.

targos · 2021-08-12T08:31:54Z

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

targos · 2021-08-12T08:35:43Z

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.

guybedford · 2021-08-12T14:42:31Z

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

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 previously requested changes Aug 11, 2020

View reviewed changes

jasnell reviewed Aug 11, 2020

View reviewed changes

src/node_options.cc Show resolved Hide resolved

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

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

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Ayase-252 mentioned this pull request Jun 5, 2021

[Feature Request]: Add promise resolving in REPL #38921

Closed

aduh95 mentioned this pull request Jun 24, 2021

repl: unflag --experimental-repl-await #39142

Closed

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

This comment has been minimized.

Sign in to view

guybedford approved these changes Jul 8, 2021

View reviewed changes

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 force-pushed the unflag-tla-repl branch from 503c640 to a9d6eb4 Compare July 8, 2021 18:37

This comment has been minimized.

Sign in to view

ejose19 mentioned this pull request Jul 14, 2021

REPL is evaluating new lines before finishing execution of previous ones #39387

Open

fixup: alphabetic order of flags.

a9f1e00

guybedford closed this Jul 16, 2021

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

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

BethGriggs mentioned this pull request Jul 26, 2021

v16.6.0 proposal #39534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: unflag Top-Level Await #34733

repl: unflag Top-Level Await #34733

hemanth commented Aug 11, 2020

devsnek left a comment

BridgeAR commented Aug 11, 2020 •

edited

Loading

devsnek commented Aug 11, 2020

hemanth commented Aug 11, 2020

BridgeAR commented Aug 11, 2020

hemanth commented Aug 11, 2020

hemanth commented Aug 11, 2020

hemanth commented Aug 12, 2020

guybedford commented Jul 2, 2021

hemanth commented Jul 6, 2021

guybedford commented Jul 7, 2021

hemanth commented Jul 7, 2021

This comment has been minimized.

guybedford left a comment

hemanth commented Jul 8, 2021

guybedford commented Jul 8, 2021

hemanth commented Jul 8, 2021

This comment has been minimized.

hemanth commented Jul 15, 2021 •

edited

Loading

nodejs-github-bot commented Jul 15, 2021

nodejs-github-bot commented Jul 16, 2021

guybedford commented Jul 16, 2021

hemanth commented Jul 16, 2021

cspotcode commented Jul 16, 2021

guybedford commented Jul 16, 2021

cspotcode commented Jul 16, 2021 •

edited

Loading

guybedford commented Jul 16, 2021

cspotcode commented Jul 16, 2021

aduh95 commented Aug 12, 2021

targos commented Aug 12, 2021 •

edited

Loading

targos commented Aug 12, 2021

guybedford commented Aug 12, 2021

repl: unflag Top-Level Await #34733

repl: unflag Top-Level Await #34733

Conversation

hemanth commented Aug 11, 2020

devsnek left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 11, 2020 • edited Loading

devsnek commented Aug 11, 2020

hemanth commented Aug 11, 2020

BridgeAR commented Aug 11, 2020

hemanth commented Aug 11, 2020

hemanth commented Aug 11, 2020

hemanth commented Aug 12, 2020

guybedford commented Jul 2, 2021

hemanth commented Jul 6, 2021

guybedford commented Jul 7, 2021

hemanth commented Jul 7, 2021

This comment has been minimized.

guybedford left a comment

Choose a reason for hiding this comment

hemanth commented Jul 8, 2021

guybedford commented Jul 8, 2021

hemanth commented Jul 8, 2021

This comment has been minimized.

hemanth commented Jul 15, 2021 • edited Loading

nodejs-github-bot commented Jul 15, 2021

nodejs-github-bot commented Jul 16, 2021

guybedford commented Jul 16, 2021

hemanth commented Jul 16, 2021

cspotcode commented Jul 16, 2021

guybedford commented Jul 16, 2021

cspotcode commented Jul 16, 2021 • edited Loading

guybedford commented Jul 16, 2021

cspotcode commented Jul 16, 2021

aduh95 commented Aug 12, 2021

targos commented Aug 12, 2021 • edited Loading

targos commented Aug 12, 2021

guybedford commented Aug 12, 2021

BridgeAR commented Aug 11, 2020 •

edited

Loading

hemanth commented Jul 15, 2021 •

edited

Loading

cspotcode commented Jul 16, 2021 •

edited

Loading

targos commented Aug 12, 2021 •

edited

Loading