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

cli: rename --loader to --experimental-loader #29752

Closed

Conversation

reasonablytall
Copy link
Contributor

Renames the --loader cli argument to --experimental-loader. This is
to clearly indicate the esm loader feature as experimental even after
esm is no longer experimental.

Also minorly alters the --experimental-loader docs to say that the
passed loader can be an esm module.

An alternative to this PR is to leave things unchanged and continue to use --experimental-modules to allow the use of --loader, even after ECMAScript modules are unflagged. This would reduce the number of different argument schemes a hypothetical script that wraps node would need to try, as discussed here: nodejs/modules#351 (comment)

Refs: nodejs/modules#351 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 28, 2019
@addaleax addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 28, 2019
@addaleax
Copy link
Member

Is the intention to go back to using --loader once ESM is no longer experimental? If so – and maybe also if not – I’d recommend providing --loader as an (for-now-)undocumented alias via AddAlias().

@guybedford
Copy link
Contributor

@addaleax the intention is to unflag modules while leaving loader as experimental for the time being, unflagging only after the development work to bring loaders to stable. Whether this could happen for 12 or only in 13 is another question too, and one we were hoping to get some feedback from on the releases group side.

@addaleax
Copy link
Member

@guybedford Okay, then replace “once ESM is no longer experimental” with “once ESM loaders are no longer experimental” :)

@reasonablytall
Copy link
Contributor Author

Using --experimental-loader prints an experimental warning message, so having --loader as an undocumented alias seems reasonable to me. I'll make the change.

The check is failing because I use cli as a subsystem. I'll change that to src since node_options.cc is the most affected.

@addaleax
Copy link
Member

The check is failing because I use cli as a subsystem. I'll change that to src since node_options.cc is the most affected.

I would ignore that, tbh. cli or esm are both better choices than src, imo.

Renames the `--loader` cli argument to `--experimental-loader`.  This is
to clearly indicate the esm loader feature as experimental even after
esm is no longer experimental.

Also minorly alters the `--experimental-loader` docs to say that the
passed loader can be an esm module.

Refs: nodejs/modules#351 (comment)
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 28, 2019
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the cli Issues and PRs related to the Node.js command line interface. label Sep 28, 2019
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

I think doc/node.1 needs to be updated as well.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Oct 1, 2019

Landed in 15fb02a

@Trott Trott closed this Oct 1, 2019
Trott pushed a commit that referenced this pull request Oct 1, 2019
Renames the `--loader` cli argument to `--experimental-loader`.  This is
to clearly indicate the esm loader feature as experimental even after
esm is no longer experimental.

Also minorly alters the `--experimental-loader` docs to say that the
passed loader can be an esm module.

Refs: nodejs/modules#351 (comment)

PR-URL: #29752
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Oct 1, 2019
Renames the `--loader` cli argument to `--experimental-loader`.  This is
to clearly indicate the esm loader feature as experimental even after
esm is no longer experimental.

Also minorly alters the `--experimental-loader` docs to say that the
passed loader can be an esm module.

Refs: nodejs/modules#351 (comment)

PR-URL: #29752
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants