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.

Sign up for GitHub

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

repl: unflag Top-Level Await #34733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a9d6eb4
repl: unflag top-level await
hemanth Jul 8, 2021
75d5b8e
repl: adding --no-experimental-repl-await flag.
hemanth Jul 13, 2021
4c89088
Update doc/api/cli.md
hemanth Jul 13, 2021
1aafad6
Update doc/api/cli.md
hemanth Jul 13, 2021
e5c977f
Update src/node_options.cc
hemanth Jul 13, 2021
fda3e7b
Update test/parallel/test-process-env-allowed-flags-are-documented.js
hemanth Jul 13, 2021
b9fced9
Update src/node_options.cc
hemanth Jul 13, 2021
b32662d
fixup: lint fix, link order.
hemanth Jul 13, 2021
a11339b
fixup: using the placeholder REPLACEME
hemanth Jul 13, 2021
a9f1e00
fixup: alphabetic order of flags.
hemanth Jul 14, 2021
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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