feat: add REPL top level await support

[ejose19]

EDIT from @cspotcode:
linking to the issues being closed by this PR
Implements and closes #245

For my records, was added to node here
Opt-out flag documented here

---

This is a port of node own REPL top level await mechanism, however it's still not fully working:

• calling a single statement with await works as expected, however typing a second statement result is an error SyntaxError: await is only valid in async functions and the top level bodies of modules even if the second statement is not using await, example:

> await new Promise((r) => setTimeout(() => r(1), 2000));
1
> 1 + 1
/home/user/<repl>.ts:1
    await new Promise((r) => setTimeout(() => r(1), 2000));
    ^^^^^

Uncaught:
SyntaxError: await is only valid in async functions and the top level bodies of modules

• variable usage results in error (although the promise is resolving correctly), may be related to Top-level await support in REPL #245 (comment)

> const x = await new Promise((r) => setTimeout(() => r(1), 2000));
Uncaught ReferenceError: x is not defined

Other notes:

• as seen in compile comment, it's necessary to decide if this functionability should be enabled using a ts-node flag or check if node --experimental-repl-await is set
• we may want to use more logic of https://github.com/nodejs/node/blob/975bbbc443c47df777d460fadaada3c6d265b321/lib/repl.js, specifically to conditional awaiting the result and wrapped cmd retries.

[ejose19]

@cspotcode Could you please review if this is the right approach?

[ejose19]

Ok, upon inspecting I know why issue 1 was happening, it's due:

ts-node/src/repl.ts

Lines 233 to 235 in 5643ad6

	return changes.reduce((result, change) => {
	return change.added ? exec(change.value, state.path) : result;
	}, undefined);

For TLA, where the code it's wrapped in async IIFE, a single await cannot be exec without its wrapper or it will be return "await is only valid in async functions and the top level bodies of modules" error.

So what I adjusted is to exec all the new code instead of each change individually if TLA was detected, and now it works without issues.

For 2nd issue, I've checked the output (compiled) code:
intput:

await 1;
const x = await 2;

output:

"use strict";
(async () => {
    await 1;
    void (x = await 2);
})();

this is a bit more puzzling, I don't know why the compiler is removing the const part there.

EDIT: I found the cause with 2nd issue, and it's an upstream nodejs bug, it can be reproduced with:

node --use_strict --experimental-repl-await

> const x = await 1;
Uncaught ReferenceError: x is not defined
    at REPL1:1:24
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

Problematic code is here:
https://github.com/nodejs/node/blob/c2e6822153bad023ab7ebd30a6117dcc049e475c/lib/internal/repl/await.js#L53-L60

Since ts-node REPL is always in strict mode, the issue was more visible on this side. I've completely removed that substitution and now all kind of variables works correctly, but a generic fix may involve having that function know if the code will run under strict mode to appropriately apply substitution.

[ejose19]

This PR is now in a fully working state, but we may want until nodejs/node#39265 is merged upstream since this PR is using the same fix.

Meanwhile, it's still needed to decide how to enable this feature (i'm leaning more towards a ts-flag since the implementation was made on this side).

Also, I'd like to add this test suite https://github.com/nodejs/node/blob/master/test/parallel/test-repl-preprocess-top-level-await.js, but don't know where exactly should I add it.

[ejose19]

Since there are many changes in upstream node PR, I'll mark this as draft until it's merged, so I can update this PR based on final version.

[cspotcode]

@ejose19 thanks for writing this! I've been on vacation so it will be a few more days till I have time to review this.

If node has an upstream bugfix, I would say it's ok for us to release a version of ts-node with that bugfix ahead of node.

[codecov]

Codecov Report

Merging #1383 (fae10d4) into main (08585b9) will increase coverage by 0.86%.
The diff coverage is 77.72%.

Impacted Files	Coverage Δ
src/bin.ts	90.69% <50.00%> (-0.71%)	⬇️
dist-raw/node-primordials.js	61.90% <66.66%> (+1.90%)	⬆️
src/repl.ts	82.41% <74.31%> (+6.35%)	⬆️
src/index.ts	77.74% <76.47%> (-0.12%)	⬇️
dist-raw/node-repl-await.js	82.97% <82.97%> (ø)
src/configuration.ts	93.82% <100.00%> (ø)
src/transpilers/swc.ts	73.80% <0.00%> (-1.20%)	⬇️

[cspotcode]

Meanwhile, it's still needed to decide how to enable this feature (i'm leaning more towards a ts-flag since the implementation was made on this side).

You said that node enables it via --experimental-repl-await, right? We already read certain node CLI flags, for example --experimental-specifier-resolution=node. I suggest we do the same here, because we generally try to behave like a drop-in replacement for node, at least behaviorally.

Also, I'd like to add this test suite https://github.com/nodejs/node/blob/master/test/parallel/test-repl-preprocess-top-level-await.js, but don't know where exactly should I add it.

How long will it take to run? Will it slow down our tests much? All our tests are declared in src/test/; can you add it there?

Our tests currently pack ts-node into a tarball, then install the tarball into ./tests/node_modules. This means tests can run the ts-node CLI and require('ts-node'). Can that test suite be adapted to do the same? If so, we can run it in parallel to our other tests.

[ejose19]

Meanwhile, it's still needed to decide how to enable this feature (i'm leaning more towards a ts-flag since the implementation was made on this side).

You said that node enables it via --experimental-repl-await, right? We already read certain node CLI flags, for example --experimental-specifier-resolution=node. I suggest we do the same here, because we generally try to behave like a drop-in replacement for node, at least behaviorally.

Yes, if that's the case it should be documented that the flag doesn't rely on node own TLA mechanism, but instead a based-mechanism, also I think an alias would be very useful since TLA has a lot of relevancy when used in REPL (as writting NODE_OPTIONS='--experimental-top-level-await' ts-node is way too large for a REPL mode that may end used frequently)

Also, I'd like to add this test suite https://github.com/nodejs/node/blob/master/test/parallel/test-repl-preprocess-top-level-await.js, but don't know where exactly should I add it.

How long will it take to run? Will it slow down our tests much? All our tests are declared in src/test/; can you add it there?

Our tests currently pack ts-node into a tarball, then install the tarball into ./tests/node_modules. This means tests can run the ts-node CLI and require('ts-node'). Can that test suite be adapted to do the same? If so, we can run it in parallel to our other tests.

If we're upstreaming the file, I doubt the tests will be relevant anymore, as at most we only will be changing imports. However these tests are very brief, doubt we will have any slowdown if including that and this other relevant test suite.

[cspotcode]

also I think an alias would be very useful since TLA has a lot of relevancy when used in REPL (as writting NODE_OPTIONS='--experimental-top-level-await' ts-node is way too large for a REPL mode that may end used frequently)

Ok, yeah. I have been putting the word experimental into any features that are experimental. This makes it obvious to the user that we might make breaking changes in a non-major release, which gives us the flexibility to change or remove them in the near future. I like having that flexibility to quickly fix bugs and stabilize new features.

We can call this flag experimentalReplTopLevelAwait or something like that. Users can either do ts-node --experimental-top-level-await (matching the flag's name from nodejs) or add "ts-node": {"experimentalReplTopLevelAwait": true} to their tsconfig.json. How does that sound?

EDIT: the node flag is --experimental-repl-await so we can copy that name: --experimental-repl-await on the CLI and experimentalReplAwait in tsconfig or programmatically.

If we're upstreaming the file, I doubt the tests will be relevant anymore, as at most we only will be changing imports. However these tests are very brief, doubt we will have any slowdown if including that and this other relevant test suite.

I'll leave it up to you whether or not you want to add them to our test suite. I see what you mean, they should be fast to run. Are you thinking you can copy-paste them into ./src/test/ and import and invoke them from within ./src/test/index.spec.ts?

[cspotcode]

as writting NODE_OPTIONS='--experimental-top-level-await' ts-node is way too large for a REPL mode that may end used frequently

We should keep in mind that node is already requiring users to do this. If it's so useful, why is node requiring users to do this? If we don't require the same, does that put us in a difficult position in the future? What if node removes this feature?

This is one of the reasons I feel good keeping the feature labelled as "experimental" so that we can make breaking changes to it in non-major releases.

[ejose19]

as writting NODE_OPTIONS='--experimental-top-level-await' ts-node is way too large for a REPL mode that may end used frequently

We should keep in mind that node is already requiring users to do this. If it's so useful, why is node requiring users to do this? If we don't require the same, does that put us in a difficult position in the future? What if node removes this feature?

This is one of the reasons I feel good keeping the feature labelled as "experimental" so that we can make breaking changes to it in non-major releases.

I mean, I'm ok with just --experimental-top-level-await, the problem is that users would need to also put it under NODE_OPTIONS which becomes more verbose.

[cspotcode]

I mean, I'm ok with just --experimental-top-level-await, the problem is that users would need to also put it under NODE_OPTIONS which becomes more verbose.

You're talking about the difference between node --experimental-repl-await and NODE_OPTIONS=--experimental-repl-await ts-node or node --experimental-repl-await --loader ts-node/esm? Yeah, fair point.

[ejose19]

You're talking about the difference between node --experimental-repl-await and NODE_OPTIONS=--experimental-repl-await ts-node

Yes, this one, after all await usage in ts-node REPL, even if experimental, may become one of the most used flags since it allows to use REPL in a more natural way.

[ejose19]

The diagnostics raised here actually affect the emit.
https://www.typescriptlang.org/play?target=5#code/PTAEHUAsE9QEwPagC6QKYGc2jQJ1wrhqAgMakCuuA-ALABQAhgO6MCWyoAFAIwA0oAEwBKANwMW7TgG1+QgLrj6pBADsMnAN7QAvqAC8oSR1DaAXEJ1KV6rdEF7Dxzl22CLDsQxsbTAL0d-DytvNV9NRkCuZ24I4OEvegkMaFVSUAAzCjTkNjVQAFt2VS5hUwZQSqNWE14BESUq6qlQWXrFCqqfO0CY80tGrrC7BwNm2rd4wcru-0DNP2Dp0FmIqJjXRnjEnQYGEFAAFQQABwBaABs0ADc0C-HONmI0VUYAIyu4UNtQN7GYniCADMoiAA

Can we avoid them by forcing the REPL to start with export {};?

Sure, I agree to remove diagnostics that are not specific to TLA (like that one), could be done inside startRepl

[ejose19]

Let me know if I should take care of remaining issues, so we don't conflict on changes.

[ejose19]

Same way to suppress output as "use strict".

[cspotcode]

Let me know if I should take care of remaining issues, so we don't conflict on changes.

Sounds good. I'm still pushing changes; I'll tell you when I'm done.

[ejose19]

For the test that is failing with: 'exports is not defined', either not running delete globalInRepl.exports; or pass {createReplOpts: {forceToBeModule: false}} to createReplViaApi. The other 4 just need their line bumped for stackTest from 1 to 2 (due the implicit export {}).

[cspotcode]

For the test that is failing with: 'exports is not defined', either not running delete globalInRepl.exports; or pass {createReplOpts: {forceToBeModule: false}} to createReplViaApi.

This should be covered by exec('exports = module.exports', ....), right?
https://github.com/ejose19/ts-node/blob/781a886cd9bb75039bd023077bcd6c773214d98a/src/repl.ts#L462-L463

The other 4 just need their line bumped for stackTest from 1 to 2 (due the implicit export {}).

I'm done for today. Feel free to push fixes for those issues.

[ejose19]

For the test that is failing with: 'exports is not defined', either not running delete globalInRepl.exports; or pass {createReplOpts: {forceToBeModule: false}} to createReplViaApi.

This should be covered by exec('exports = module.exports', ....), right?
https://github.com/ejose19/ts-node/blob/781a886cd9bb75039bd023077bcd6c773214d98a/src/repl.ts#L462-L463

Not right now, as if (forceToBeModule) state.input += ';export {};void 0;\n'; is defined in createRepl and that exec runs in startRepl, while the tests evals code in between. So either we must remove the delete for globalInRepl.module & globalInRepl.exports in tests or execute the ';export {};void 0;\n'; line in another exec below exec('exports = module.exports', state.path, context);

EDIT: I've opted for the 2nd option, as we're only interested in doing this for interactive repl due TLA support (no longer need to set forceToBeModule=false in bin)

[cspotcode]

I have finally merged this. Thanks again for the great work on this pull request @ejose19 !

[ejose19]

@cspotcode You're welcome! Glad I could contribute back to this great project, and thanks for your time to review and co-author this PR

[cefn]

This is the PR of the decade! Thanks to everyone involved.