src: reserve node run command

[anonrig]

This pull-request reserves node run command.

cc @nodejs/tsc

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[ShogunPanda]

LGTM!

@anonrig Non blocking proposal: do you think you can generalize this to work on a list of commands so that in the future similar PRs will be way faster?

[mcollina]

lgtm

[anonrig]

@ShogunPanda I can follow up. Do you have a list in mind?

[ShogunPanda]

To be honest no.
But I was thinking that since we are introducing the breaking change for the CLI arg I would make it generic.
This way we deprecate the entire node script-without-extension once and for all and users will start being accustomed to the new ways of being.

[GeoffreyBooth]

@ShogunPanda I can follow up. Do you have a list in mind?

node test comes to mind. @nodejs/test_runner

[anonrig]

@GeoffreyBooth maybe node watch?

[juliangruber]

maybe node eval?

[nodejs-github-bot]

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

[anonrig]

cc @tniessen please review.

I thought that #52190 had already been merged

[tniessen]

For the same reason explained in #52190 (review): subcommands are great, but we shouldn't add one in a breaking manner without having discussed a strategy for subcommands in a much more general setting. Accepting positional arguments and subcommands in the same position is an anti-pattern, and we shouldn't make an ad-hoc decision for any specific subcommand. (On a side note, as explained elsewhere, node run is much more likely to break things than node inspect was seven years ago.)

[H4ad]

I'm not blocking but I also agree with @tniessen that we should discuss a little bit more about how we want to introduce the subcommand since is a breaking change and also a paradigm shift from "flags by default".

I also agree with @ShogunPanda, instead of introducing a breaking change every time we want to move a flag to a subcommand, we can think of possible subcommand (even if we didn't implement it in the future, like bench, compile, etc...) and then introduce all of them at once.

Maybe we also could include a cli flag or an environment variable to disable the subcommand, this could be useful for cloud environments to keep the old behavior without the fear of having to remember/know all the subcommand that we want to introduce in the future, for them, subcommand could be dangerous.

But in general, I'm very happy to see subcommand being introduced, this is a huge DX improvement!

[gireeshpunathil]

this is great for sure, but agree with @tniessen for having a strategy around subcommands before proceeding?

[GeoffreyBooth]

For the same reason explained in #52190 (review): subcommands are great, but we shouldn’t add one in a breaking manner without having discussed a strategy for subcommands in a much more general setting.

All that this PR does is add a placeholder that errors; is this really “adding” a subcommand? I agree it would be good to discuss and come up with an overall design for subcommands before we go forth and add them, but is all of that necessary just before creating the placeholder stub? It would be nice to not have to wait until 23 to land subcommands, if that’s what we decide to do (and we can always remove the stub at any time, in 22.x or 23).

[tniessen]

All that this PR does is add a placeholder that errors; is this really “adding” a subcommand?

It breaks existing applications and introduces an unforeseen edge case to the CLI, regardless of what the subcommand does.

[GeoffreyBooth]

It breaks existing applications and introduces an unforeseen edge case to the CLI

But it's a semver major change, that's the point? And the remediation is to just add a file extension, so it's not a hard break. The error message should perhaps suggest running as node run.js when such a file exists, would that address the concern?

I don't think we should shy away from breaking changes even in a major release, especially when users can mitigate so easily.

[tniessen]

It breaks existing applications and introduces an unforeseen edge case to the CLI

But it's a semver major change, that's the point?

I don't think the point of semver-major releases is to arbitrarily break things. Also, breakage aside, I still believe that mixing subcommands and positional arguments is an anti-pattern and that we need a proper strategy for doing so.

I don't think we should shy away from breaking changes even in a major release, especially when users can mitigate so easily.

I do think we should shy away from making ad hoc design decisions leading to breaking changes to our CLI, especially because users rely on it so heavily.

[ShogunPanda]

It breaks existing applications and introduces an unforeseen edge case to the CLI

But it's a semver major change, that's the point?

I don't think the point of semver-major releases is to arbitrarily break things. Also, breakage aside, I still believe that mixing subcommands and positional arguments is an anti-pattern and that we need a proper strategy for doing so.

We might need a proper strategy, I agree on that.
But on the other side I also agree that allowing extension-less script run is an antipattern as well (never used in 10 years in Node and never liked that, but that's just my personal opinion here).

I don't think we should shy away from breaking changes even in a major release, especially when users can mitigate so easily.

I do think we should shy away from making ad hoc design decisions leading to breaking changes to our CLI, especially because users rely on it so heavily.

Heavily? Do you have any poll or number on thing? I'm genuinely curious.

I trust you on the numbers so accept them. But, despite of that, we have rationale for that change as people proposed this might open for future subcommand. And, to be honest, subcommands in CLI app is definitely not an antipattern and widely used. We can't stagnate development and innovation because users might like the old DX, especially since there is an easy way to migrate.

I strongly pro this.

[ShogunPanda]

After writing the message above I just thought a thing that might be a good compromise.

Let's say the user runs node $ARG (where $ARG is anything not containing a dot).
We proceed like that:

1. In v22 we run $ARG.js if it exists but we show a warning that in v23 this behavior will go away.
2. If the file above $ARG.js does not exist (or if the user specify a new CLI flag to bypass the file) and node $ARG (for instance node run) has been implemented we execute the function.
3. In v23 the old behavior is completely removed and node $ARG will only run the $ARG functionality if it exists.

This way we can start reserving the behavior for user that want to use subcommands and, at the same time, not interfere with current users behavior and start warn them to migrate away.

What do you folks thing about it?

[richardlau]

I think this should go through a deprecation cycle.

[ShogunPanda]

I think this should go through a deprecation cycle.

I think we wrote at the same time. Do you mean the original idea or my proposal?

[ronag]

I think moving away from automatically inferring extensions would be a good thing.

[richardlau]

I think this should go through a deprecation cycle.

I think we wrote at the same time. Do you mean the original idea or my proposal?

I mean any change to how node run behaves (irrespective of what the new behaviour is).

[targos]

We might need a proper strategy, I agree on that.
But on the other side I also agree that allowing extension-less script run is an antipattern as well (never used in 10 years in Node and never liked that, but that's just my personal opinion here).

It doesn't have to be extension-less. node run will execute run.js if it exists.

[tniessen]

our CLI, especially because users rely on it so heavily.

Heavily? Do you have any poll or number on thing? I'm genuinely curious.

I meant the established CLI conventions in general, i.e., I assume that the vast majority of users is relying on the fact that node will attempt to execute whatever positional argument comes first as a JavaScript file, with the unfortunate exception of node inspect.

No, I do not have any poll for how many users use the Node.js CLI.

[joyeecheung]

I think starting with a warning is a good idea (note that warning is also semver-major).

But it's a semver major change, that's the point? And the remediation is to just add a file extension, so it's not a hard break.

I think we are mostly guessing how users use the CLI here, and it's difficult to estimate the breakage without a warning. If it turned out that there were use cases we didn't anticipate and couldn't fix, reverting a warning would be a lot easier than reverting an actual breaking change.

[benjamingr]

If the objection is to node run then would node --run=scriptName <*rest of args passed to script*> work?

(Similar to --test for example though most flags do not change execution modes but instead add attributes like --watch or --inspect-brk)

[MoLow]

I think we should have a discussion in the TSC about subcommands in general.
we currently avoid adding too many CLI flags and supporting sub-commands can reduce the noise of flags that are specific to a sub-command
additionally, a lot of flags specific to the test runner are very verbose - if node --test --test-timeout 1000 --test-reporter spec can be replaced with node test --timeout 1000 --reporter spec that will be great

[tniessen]

If the objection is to node run then would node --run=scriptName <*rest of args passed to script*> work?

Yes, I believe that's what @joyeecheung proposed in an earlier PR.

additionally, a lot of flags specific to the test runner are very verbose

Precisely. Subcommands are a great way to reduce the number of "global" CLI options, but I've definitely relied on node test running test.js as a regular CommonJS file in the past, so I have to assume it's not entirely uncommon. Each subcommand has the potential to break things, so having a discussion about a general strategy instead of introducing them on a case-by-case basis seems reasonable to me.