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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -279,12 +279,11 @@ added: v11.8.0

Use the specified file as a security policy.

### `--experimental-repl-await`
### `--no-experimental-repl-await`
<!-- YAML
added: v10.0.0
-->

Enable experimental top-level `await` keyword support in REPL.
added: REPLACEME
-->
Use this flag to disable top-level await in REPL.

### `--experimental-specifier-resolution=mode`
<!-- YAML
Expand Down Expand Up @@ -1401,7 +1400,6 @@ Node.js options that are allowed are:
* `--experimental-loader`
* `--experimental-modules`
* `--experimental-policy`
* `--experimental-repl-await`
* `--experimental-specifier-resolution`
* `--experimental-top-level-await`
* `--experimental-vm-modules`
Expand All @@ -1423,6 +1421,7 @@ Node.js options that are allowed are:
* `--max-http-header-size`
* `--napi-modules`
* `--no-deprecation`
* `--no-experimental-repl-await`
* `--no-force-async-hooks-checks`
* `--no-warnings`
* `--node-memory-debug`
Expand Down
7 changes: 4 additions & 3 deletions doc/api/repl.md
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,7 @@ Error: foo

#### `await` keyword

With the [`--experimental-repl-await`][] command-line option specified,
experimental support for the `await` keyword is enabled.
Support for the `await` keyword is enabled at the top level.

```console
> await Promise.resolve(123)
Expand Down Expand Up @@ -250,6 +249,8 @@ undefined
234
```

[`--no-experimental-repl-await`][] shall disable top-level await in REPL.

### Reverse-i-search
<!-- YAML
added:
Expand Down Expand Up @@ -764,7 +765,7 @@ For an example of running a REPL instance over [`curl(1)`][], see:
[TTY keybindings]: readline.md#readline_tty_keybindings
[ZSH]: https://en.wikipedia.org/wiki/Z_shell
[`'uncaughtException'`]: process.md#process_event_uncaughtexception
[`--experimental-repl-await`]: cli.md#cli_experimental_repl_await
[`--no-experimental-repl-await`]: cli.md#cli_no_experimental_repl_await
[`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`]: errors.md#errors_err_domain_cannot_set_uncaught_exception_capture
[`ERR_INVALID_REPL_INPUT`]: errors.md#errors_err_invalid_repl_input
[`curl(1)`]: https://curl.haxx.se/docs/manpage.html
Expand Down
6 changes: 2 additions & 4 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,8 @@ to use as a custom module loader.
.It Fl -experimental-policy
Use the specified file as a security policy.
.
.It Fl -experimental-repl-await
Enable experimental top-level
.Sy await
keyword support in REPL.
.It Fl -no-experimental-repl-await
Disable top-level await keyword support in REPL.
.
.It Fl -experimental-specifier-resolution
Select extension resolution algorithm for ES Modules; either 'explicit' (default) or 'node'.
Expand Down
3 changes: 2 additions & 1 deletion lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ const {
validateFunction,
validateObject,
} = require('internal/validators');

const experimentalREPLAwait = getOptionValue(
'--experimental-repl-await'
);
Expand Down Expand Up @@ -421,6 +420,8 @@ function REPLServer(prompt,
wrappedCmd = true;
}

// `experimentalREPLAwait` is set to true by default.
// Shall be false in case `--no-experimental-repl-await` flag is used.
if (experimentalREPLAwait && StringPrototypeIncludes(code, 'await')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have now a mechanism to flip the default value of a flag (making it possible to opt out with --no-experimental-repl-await.
Are we confident enough about the feature to remove the flag?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does sound like a good way to ensure an opt-out in case there are issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, do we introduce --no-experimental-repl-await flag and check if it not set here or we use the ImpliesNot("--no-harmony-top-level-await", "--experimental-top-level-await");? Either way we would need to getOptionValue and check if it is not set in this condition, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hemanth I believe just the ImpliesNot line is enough here, and then changing the default value for --experimental-repl-await to being enabled. That should fully cover the feature per my reading of #39023.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, so experimental-repl-await shall be undocumented and enabled, --no-experimental-repl-await shall be documented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the change! I added a few suggestions

if (processTopLevelAwait === undefined) {
({ processTopLevelAwait } = require('internal/repl/await'));
Expand Down
3 changes: 2 additions & 1 deletion src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,8 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
AddOption("--experimental-repl-await",
"experimental await keyword support in REPL",
&EnvironmentOptions::experimental_repl_await,
kAllowedInEnvironment);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
kAllowedInEnvironment,
true);
AddOption("--experimental-vm-modules",
"experimental ES Module support in vm module",
&EnvironmentOptions::experimental_vm_modules,
Expand Down
2 changes: 1 addition & 1 deletion src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class EnvironmentOptions : public Options {
std::string experimental_policy;
std::string experimental_policy_integrity;
bool has_policy_integrity_string;
bool experimental_repl_await = false;
bool experimental_repl_await = true;
bool experimental_vm_modules = false;
bool expose_internals = false;
bool frozen_intrinsics = false;
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-repl-import-referrer.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const assert = require('assert');
const cp = require('child_process');
const fixtures = require('../common/fixtures');

const args = ['--interactive', '--experimental-repl-await'];
const args = ['--interactive'];
const opts = { cwd: fixtures.path('es-modules') };
const child = cp.spawn(process.execPath, args, opts);

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-repl-top-level-await.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const repl = require('repl');

common.skipIfInspectorDisabled();

// Flags: --expose-internals --experimental-repl-await
// Flags: --expose-internals

const PROMPT = 'await repl > ';

Expand Down