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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26281de
Draft one of the yarn cmds rfc
orta Feb 26, 2021
bc28fb8
Update
orta Feb 26, 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
73 changes: 73 additions & 0 deletions accepted/0000-missing-command-lookups.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# Support Yarn style command/script/bin lookups from the CLI

## Summary

Allow for a CLI experience like:

```sh
npm install typescript
npm tsc
```

## Motivation

I have two main motivations:

- 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.

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

@eyelidlessness eyelidlessness Mar 1, 2021 •
edited
Loading

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.

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`.


## Detailed Explanation

When npm receives a command it does not recognise, npm first resolves the next string to see if it matches:

- Available "scripts" in `package.json`
- Commands matching in `./node_modules/.bin/`

Failing that, then show help.

## Rationale and Alternatives

1. Having a `script`s section like this:

```json
{
"scripts": {
"tsc": "tsc",
"eslint": "eslint",
"prettier": "prettier"
}
}
```

Is a waste of space in the JSON, the `node_modules/.bin` folder is typically full of commands and after a while you either start prefixing commands with `node_modules/.bin` instead or you fill up the scripts section.

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.


- `npm tsc` does not work today, anyone who is doing that is seeing a help message.
- `npm test:lint` also does not work today.


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 •
edited
Loading

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?


## Implementation

I think the biggest question is the `--` below.

## Prior Art

Yarn does this, Berry does this and PNPM has indicated they're open to supporting it too.

## Unresolved Questions and Bikeshedding

I assume npm uses `--` to match the [unix format](https://unix.stackexchange.com/questions/11376/what-does-double-dash-mean) but I'd still advocate for it's removal here. `npm -- tsc` is a strange command to write, and the intention is quite obvious (given that npx also went with the same decision)

I assume there are flags to npm which you may want to be able to include e.g. `npm --registry-something=mything.com deploy`. Which would call `npm deploy` after those options are set. I assume that the npm client knows all of these commands ahead of time, and can either remove those before looking up the script/bin, or we can assume that any string beginning with `-` won't be a script/binary.

---

^1 - I basically just removed any traditional command, then slowly dropped anything which idn't look like it used a script or binary. Then rounded down to =assume there would be a bunch of exceptions.