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

RFC: Support Yarn style command/script/bin lookups from the CLI with npm prefix #332

Closed
wants to merge 2 commits into from

Conversation

orta
Copy link
Contributor

@orta orta commented Feb 26, 2021

References

Alternative to #279


- Personal, looking at the list of yarn commands I [use in an average month](https://gist.github.com/orta/489dbfa1b7467804955ea7800adf1494#file-last_month_yarn-txt) (1200) roughly 30% (400) of those commands are using the shorthand syntax to run a command installed into `node_modules/.bin` in a project.

- Professional, it'd be great for the TypeScript docs to be able to simply say: `npm install typescript --save-dev' then `npm tsc index.ts` and there to be no ambiguity like our current recommendation for `npx tsc index.ts`.
Copy link
Contributor

Choose a reason for hiding this comment

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

if you suggest they install it first, what ambiguity is there in npx tsc index.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep getting feedback because people assume "npx = remote", and not "local then remote", I get that they are not right, but I also thought that until I researched what we should recommend.

Copy link
Contributor

Choose a reason for hiding this comment

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

in npm 7, npx will prompt before auto-installing, so as npm 7 propagates, more people hopefully will have their expectations corrected.

Copy link

Choose a reason for hiding this comment

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

I also only learned recently that npx prefers local resolution. I’d avoided using npx from the start because it makes me super nervous to run remote code (I’m also “that guy” who warns people not to pipe to curl even if you read the source because it can be detected server side).

Now knowing about local resolution, I’ll use npx in projects where I don’t choose the package manager, but only in the scripts section where I’m prompted to also verify there’s a local dependency. And even then it makes me super nervous that the dependency will be removed and introduce a silent vulnerability. So nervous that anytime I go to use an npx command and I know my git HEAD has changed I try to remember to check the package.json, and I just know someday I’m gonna forget. Not a great UX! (I’m sure some combination of npm flags and a grep and an alias could make this safe for my preferences, but then I’m moving that risk to anyone I forget to warn!)

IMO requiring a local install, whatever the ultimate API, is an important feature for security and repeatability. And those are worth the distinction of a new API, even if TIMTOWTDI.

Edit: I meant to address the v7 behavior. I think it’s pretty likely people generally don’t know about local resolution and I would expect a proliferation of alias gists adding a --yes flag or whatever bypass to restore the old convenient behavior. It’s just a different use case and I do think it should be treated as one.


2. This change removes the concept of "blessed" scripts. `npm test` works but `npm test:lint` does not. Eventually folks memorize the ones they can use shorthand for, but there's no reason for the distinction other than at some point it was decided to be given a short-cut. These special cases can be removed with this change.

3. It does not break any existing setups.
Copy link
Contributor

Choose a reason for hiding this comment

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

if npm added an "npm build" command the release after shipping this RFC, though, it would break a great many existing setups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a possibility yes, at that point there would probably be a major npm semver and breaking changes like that can be handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right - but the consequence is that npm could no longer add top-level commands without it being a semver-major change; and regardless of semver-major allowing them to do it, that doesn't change the disruption it might cause for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO that's worth it. This behavior accounts for roughly 40% of my command line usage in yarn. I don't maintain npm, so that's just a anecdata but this feature has been a constant win for yarn from day 1. Even post npm 7 and in yarn deprecated land, I regularly recommend people to not use npm just for this alone.

There's possible mitigation strategies like warnings when they match a potential upcoming command, or just outright allowing the script to take precedence over new commands. npm could emit that this is blocking a known CLI command and recommend a rename while still running the same script. I am assuming under the hood a new npm publish2 isn't directly calling out to the shell with npm pack2 for example, and so adding a script pack2 wouldn't take precedence in the internals and people's projects wouldn't break.

accepted/0000-missing-command-lookups.md Outdated Show resolved Hide resolved

Alternative to:

- [RFC 279:for --default-command](https://github.com/npm/rfcs/pull/279) - tries to hit the same issue, but because it'd a user option it wouldn't be useful at the eco-system level and only people who know it exists would use it.
Copy link
Contributor

Choose a reason for hiding this comment

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

The way I see it we can still have the option proposed in the default-command RFC. It would allow us to ship the ability to swap your default-command in npm7 and when npm8 comes we can simply turn the default value to default-command=run-script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that could definitely work if run-script was expanded to also include binaries 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, that would be a diff command then!

Copy link
Contributor

@ljharb ljharb Mar 1, 2021

Choose a reason for hiding this comment

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

It seems like pursuing a new command that combined "exec without any option to install" and "fall back to run-script", and then seeing how useful that command is when set to the default-command, would be a viable and appropriately cautious path forward here?

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Feb 27, 2021
@isaacs
Copy link
Contributor

isaacs commented Mar 1, 2021

I agree that this behavior is convenient, and I like convenience. That said, there are a few footguns and drawbacks that we identified in the --default-command RFC which would have to be addressed to move forward on this:

First, it makes the addition of any new command semver-major. We could either decide that this is fine, effectively locking the set of top-level commands for v7, or we could keep track of which commands were introduced since this change, and make those commands prefer the script/bin with a warning, and switch them over in the next major. That's a complexity cost, but not a huge one. The bigger impact of this is that we will have to be careful to introduce new commands even in a semver-major release, because breaking changes tend to be obstacles to users upgrading, and we work very hard to make upgrades as easy as possible.

Second, running scripts is one thing, but running any binary in node_modules/.bin is a bit more risky. For example, let's say that we add a new npm foo command, but mark it as "still preferring the script" for now. Then, in a semver-minor release, one of your dependencies introduces a foo binary, breaking your build by taking over the top-level npm command. In this case, a user has done nothing but reasonable semver-minor upgrades, and find themselves in a semver-major situation unexpectedly. So that's not good.

I think that would mean that not only do we have to search for existing scripts on the registry, but also for existing bins on the registry, to ensure that we're not breaking anyone by adding a new command, and even there, we can't be sure who's using the shorthand and who isn't. So any command addition is effectively semver-major no matter what, and potentially a build-breaking addition that we will have a hard time predicting. (We'd probably want to back-port a list of new commands, so that npm v7 could warn about those if it sees them in the package's bin or scripts blocks.)

I would be totally fine with adding a npm exec-local command which does not install anything and only looks in the local node_modules/.bin contexts. That would be very easy. Fun shorthand, too npm xl ;)

The resolution of the --default-command RFC discussion was to upgrade help-search to also search bins and scripts, and suggest the appropriate command to run.

@orta
Copy link
Contributor Author

orta commented Mar 2, 2021

Yeah, I agree with all of your points ( though scouring the entire registry feels a tad drastic but I understand that is probably the only way to be truly certain thread of time )

I haven't been able to think of a more elegant a way to handle the tension between allowing for new npm commands and having perfect backwards compatability either. I'll still keep it in mind, but I'm not sure if there is a perfect answer out there to find, and so I'll keep thinking around chipping down the trade-offs

For example, another way to narrow down the set potential conflicts for the .bin resolution strategy is to have it work like Yarn's strict mode where you have to have the corresponding package as a non-transitive dependency for commands to be usable via the shorthand.

@eyelidlessness
Copy link

With so much focus on the impact of new commands in semver-minor versions, it seems like a corresponding RFC to semver addressing the fact that new commands are breaking changes may be warranted. This isn’t a theoretical problem only NPM has to consider, the behavior referenced has existed in Yarn for a long time. Updating semver to recognize the issue wouldn’t necessarily prevent it, but it would certainly raise awareness.

@ljharb
Copy link
Contributor

ljharb commented Mar 2, 2021

New commands are absolutely not breaking changes in the common case, so that would be an incorrect RFC. It is a theoretical problem for npm with this rfc, and is a long-standing real problem for yarn, but most projects haven’t made the choice yarn has, and as such, for most projects new commands are semver-minor.

@eyelidlessness
Copy link

New commands are absolutely not breaking changes in the common case

I used to feel the same way, but I was already turning a corner, and this discussion has slammed the needle to 11 for me. In the common case, iterating over all possible command/outcome cases should be considered unknown. If additive changes are considered non-breaking, then a list of known commands is considered non-exhaustive. Anything built around the tooling or documentation could be considered unpredictable for unknown commands at best.

This is all probably a stronger argument against Yarn-style behavior, but it’s so common that people already safeguard against it.

In a semver scenario the equivalent is breaking any Object.keys/comparable expectations. This is why TypeScript can’t type that method. Adding new functionality seems safe until you consider how people depend on the scope of existing functionality.

That objection either means redirecting this RFC to different semantics or accepting that new commands are, in fact, breaking changes.

@ljharb
Copy link
Contributor

ljharb commented Mar 3, 2021

It is indeed a strong argument against yarn-style behavior; and i see no evidence anyone is protecting against it.

Additive changes are not breaking, full stop. If you’re doing reflection and relying on the absence of a property, you’re voiding the warranty.

@eyelidlessness
Copy link

That’s a somewhat surprising response. So far the strongest resistance to the behavior has been the risk of additive behavior breaking existing workflows, a tacit acknowledgment that additive changes are in fact breaking changes.

The claim no one is protecting against it seems to suggest that either Yarn, having long had the behavior, has either experienced such breakage, or has just had dumb luck not experiencing it. For my usage I’ve avoided script names that would be ambiguous. I’d even do so without Yarn’s behavior because ambiguity is a mental tax. I think this is common not just in Yarn usage but in NPM/PNPM usage as well.

@ljharb
Copy link
Contributor

ljharb commented Mar 3, 2021

@eyelidlessness adding new top-level commands to a CLI is safe in general; it isn’t safe for yarn, and wouldn’t be safe for npm if this RFC ships as-is. I assume yarn has either chosen to not add new top-level commands, or, has chosen to endure the breakage.

Any word that is a likely new top-level npm or yarn command is one I’d wager is a commonly used script name - “build”, for example. This RFC changes the decision to add a new command from “what’s a good name?” to “let’s exhaust our resources to figure out if the best names are already used, and even then it’ll probably break an enterprise codebase”.


- Personal, looking at the list of yarn commands I [use in an average month](https://gist.github.com/orta/489dbfa1b7467804955ea7800adf1494#file-last_month_yarn-txt) (1200) roughly 30% (400 (^1)) of those commands are using the shorthand syntax to run a command installed into `node_modules/.bin` in a project.

- Professional, it'd be great for the TypeScript docs to be able to simply say: `npm install typescript --save-dev' then `npm tsc index.ts` and there to be no ambiguity like our current recommendation for `npx tsc index.ts`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Professional, it'd be great for the TypeScript docs to be able to simply say: `npm install typescript --save-dev' then `npm tsc index.ts` and there to be no ambiguity like our current recommendation for `npx tsc index.ts`.
- Professional, it'd be great for the TypeScript docs to be able to simply say: `npm install typescript --save-dev` then `npm tsc index.ts` and there to be no ambiguity like our current recommendation for `npx tsc index.ts`.

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Mar 17, 2021
@orta
Copy link
Contributor Author

orta commented Mar 17, 2021

The results of this RFC a few meetings along. I'll be closing up this RFC as we've kinda hit the end-point of this one in that there's not really new discussion to be had.

Running scripts/bins from a top level global e.g. npm tsc

This is looking to be a no-go. Too many future constraints on npm, and it will cause any npm CLI change to effectively be a semver major.

There are two proposals worth keeping an eye on:

Abstracting local bins and scripts

No confirmations on what this could look like due to the tension of having to know that you use:

  • npm run to do a script (npm run compile)
  • npm exec -- or npx to launch a local dep (npm exec -- tsc)

@isaacs had a good idea above with npm exec-local which could cover those two as a single command (and could maybe not have the --) - I don't know if there's an RFC for that but I can look into that if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants