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

repl: unflag Top-Level Await #34733

Closed
wants to merge 10 commits into from
Closed

Conversation

hemanth
Copy link
Contributor

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

@devsnek devsnek left a 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.

@BridgeAR
Copy link
Member

BridgeAR commented Aug 11, 2020

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 commented Aug 11, 2020

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

@hemanth
Copy link
Contributor Author

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

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

@hemanth
Copy link
Contributor Author

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
Contributor Author

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
Contributor Author

hemanth commented Aug 12, 2020

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

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

@hemanth
Copy link
Contributor Author

hemanth commented Jul 6, 2021

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

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

@hemanth
Copy link
Contributor Author

hemanth commented Jul 7, 2021

Thank you! @guybedford I have resolved the conflicts.

I shall wait for @devsnek to unblock 🤓

@nodejs-github-bot

This comment has been minimized.

Copy link
Contributor

@guybedford guybedford left a 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.

@hemanth
Copy link
Contributor Author

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

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

hemanth commented Jul 8, 2021

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

@nodejs-github-bot

This comment has been minimized.

@hemanth
Copy link
Contributor Author

hemanth commented Jul 15, 2021

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
Collaborator

@nodejs-github-bot
Copy link
Collaborator

guybedford pushed a commit that referenced this pull request Jul 16, 2021
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

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

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

hemanth commented Jul 16, 2021

Thank you! @guybedford 👏🤸‍♂️

@cspotcode
Copy link

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

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

@cspotcode
Copy link

cspotcode commented Jul 16, 2021

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

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

targos pushed a commit that referenced this pull request Jul 17, 2021
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 pull request Jul 26, 2021
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
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
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 pull request Jul 29, 2021
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
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
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
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 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 commented Aug 12, 2021

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

@targos
Copy link
Member

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

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
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
Development

Successfully merging this pull request may close these issues.

10 participants