X Tutup
The Wayback Machine - https://web.archive.org/web/20220323000852/https://github.com/nodejs/node/pull/34733
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

Jump to bottom

repl: unflag Top-Level Await #34733

Closed
wants to merge 10 commits into from
Closed

repl: unflag Top-Level Await #34733

wants to merge 10 commits into from

Conversation

hemanth
Copy link
Member

@hemanth hemanth commented Aug 11, 2020

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

@nodejs-github-bot nodejs-github-bot added c++ repl labels Aug 11, 2020
devsnek
devsnek previously requested changes Aug 11, 2020
View changes
Copy link
Member

@devsnek devsnek left a comment

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

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Aug 11, 2020 •
edited

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
Copy link
Member

@devsnek devsnek commented Aug 11, 2020

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

@hemanth
Copy link
Member Author

@hemanth hemanth commented Aug 11, 2020

@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
Copy link
Member

@BridgeAR BridgeAR commented Aug 11, 2020

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

jasnell
jasnell reviewed Aug 11, 2020
View changes
src/node_options.cc Show resolved Hide resolved
@hemanth
Copy link
Member Author

@hemanth hemanth commented Aug 11, 2020

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
Copy link
Member Author

@hemanth hemanth commented Aug 11, 2020

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
Copy link
Member Author

@hemanth hemanth commented Aug 12, 2020

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

@Ayase-252 Ayase-252 mentioned this pull request Jun 5, 2021
Closed
@aduh95 aduh95 mentioned this pull request Jun 24, 2021
Closed
@guybedford
Copy link
Contributor

@guybedford guybedford commented Jul 2, 2021

@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
Copy link
Member Author

@hemanth hemanth commented Jul 6, 2021

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

@guybedford
Copy link
Contributor

@guybedford guybedford commented Jul 7, 2021

@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
Copy link
Member Author

@hemanth hemanth commented Jul 7, 2021

Thank you! @guybedford I have resolved the conflicts.

I shall wait for @devsnek to unblock 🤓

@devsnek devsnek dismissed their stale review Jul 7, 2021

​

@nodejs-github-bot

This comment has been hidden.

Sign in to view
guybedford
guybedford approved these changes Jul 8, 2021
View changes
Copy link
Contributor

@guybedford guybedford left a comment

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
Copy link
Member Author

@hemanth hemanth commented Jul 8, 2021

image

Thanks @guybedford!

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

@guybedford
Copy link
Contributor

@guybedford guybedford commented Jul 8, 2021

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

@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
Copy link
Member Author

@hemanth hemanth commented Jul 8, 2021

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

@nodejs-github-bot

This comment has been hidden.

Sign in to view
@ejose19 ejose19 mentioned this pull request Jul 14, 2021
Open
@hemanth
Copy link
Member Author

@hemanth hemanth commented Jul 15, 2021 •
edited

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
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jul 15, 2021

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jul 16, 2021

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

guybedford added a commit that referenced this issue 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 <guybedford@gmail.com>
@guybedford
Copy link
Contributor

@guybedford guybedford commented Jul 16, 2021

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

@guybedford guybedford closed this Jul 16, 2021
@hemanth
Copy link
Member Author

@hemanth hemanth commented Jul 16, 2021

Thank you! @guybedford 👏🤸‍♂️

@cspotcode
Copy link

@cspotcode cspotcode commented Jul 16, 2021

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

@guybedford guybedford added dont-land-on-v12.x dont-land-on-v14.x labels Jul 16, 2021
@guybedford
Copy link
Contributor

@guybedford guybedford commented Jul 16, 2021

@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
Copy link

@cspotcode cspotcode commented Jul 16, 2021 •
edited

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 guybedford added notable-change semver-minor labels Jul 16, 2021
@guybedford
Copy link
Contributor

@guybedford guybedford commented Jul 16, 2021

@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
Copy link

@cspotcode cspotcode commented Jul 16, 2021

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

targos added a commit that referenced this issue 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 <guybedford@gmail.com>
BethGriggs added a commit that referenced this issue 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
Merged
BethGriggs added a commit that referenced this issue 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 <guybedford@gmail.com>
BethGriggs added a commit that referenced this issue Jul 29, 2021
@BethGriggs
2021-07-29, Version 16.6.0 (Current)
395abc1 
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 issue 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 issue 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 issue 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
Copy link
Contributor

@aduh95 aduh95 commented Aug 12, 2021

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
Copy link
Member

@targos targos commented Aug 12, 2021 •
edited

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

@targos
Copy link
Member

@targos targos commented Aug 12, 2021

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
Copy link
Contributor

@guybedford guybedford commented Aug 12, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell

@guybedford guybedford

@targos targos

@richardlau richardlau

@devsnek devsnek

Assignees
No one assigned
Labels
c++ dont-land-on-v12.x dont-land-on-v14.x notable-change repl semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
10 participants
@hemanth @BridgeAR @devsnek @guybedford @nodejs-github-bot @targos @cspotcode @aduh95 @jasnell @richardlau
X Tutup