GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||
GeezerVison is showing github.com — repl: unflag Top-Level Await by hemanth · Pull Request #34733 · nodejs/node Conversation
Contributor
Now that we have #34558 it makes sense to unflag Top-Level await for the REPL? This PR is getting rid of
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
devsnek
previously requested changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. All reactions
|
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
We should be able to get this for free once v8:10539 is fixed |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@devsnek Agree it is not related, but this is the closest we can get to it? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@hemanth it uses acorn to parse the code to wrap it in an async IIFE. That changes the way how |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
227ef06 to
f257883
Compare
|
Fixed the failing test with: How are these two related? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
f257883 to
03225c5
Compare
|
Everything is green, except |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Oh nice @guybedford was just trying to understand the difference between these two PRs. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thank you! @guybedford I have resolved the conflicts. I shall wait for @devsnek to unblock 🤓 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
Thanks @guybedford! But looks like there is a mismatch from the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@hemanth that top commit in your screenshot looks like a merge commit which is likely the issue. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.
|
Dang, right. Thanks @guybedford the latest commit should fix it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
|
Landed in 14b26e0. Thank you @hemanth for pushing this on through! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thank you! @guybedford 👏🤸♂️ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Is this considered a breaking change or no? I'm wondering in which node version(s) it will release. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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: EDIT I think I found the right docs: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
No feedback, this answers all my questions. Thank you. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
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 <[email protected]>
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
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
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Given that #39744 only happens if top-level await is used, I think it's fine. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for the pointer. Bugs on experimental features are from a process perspective a feature not a bug! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.