repl: unflag Top-Level Await

[hemanth]

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]

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]

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

[hemanth]

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

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

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]

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]

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

[guybedford]

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

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

[guybedford]

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

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]

Thanks @guybedford!

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

[guybedford]

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

[hemanth]

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

[hemanth]

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]

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

[nodejs-github-bot]

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

[guybedford]

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

[hemanth]

Thank you! @guybedford 👏🤸‍♂️

[cspotcode]

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

[guybedford]

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

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]

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

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

[aduh95]

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]

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

[targos]

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]

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