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 for --default-command #279

Closed
wants to merge 1 commit into from
Closed

RFC for --default-command #279

wants to merge 1 commit into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Nov 9, 2020

isaacs added a commit to npm/cli that referenced this pull request Nov 9, 2020
Comment on lines +83 to +88
### Alternative 2 - just make `help` know about scripts

Another alternative would be to make the current default `help-search`
behavior look at the `"scripts"` field in the current project's
`package.json` file. If the term matches there, it could suggest that the
user might be thinking of the `run-script` command.
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a good improvement regardless, and would help the default/common case.

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, it's somewhat orthogonal to this RFC, and something we could just do regardless.

Comment on lines +120 to +127
- `npm config set default-command exec` This turns npm into something like
a more powerful `npx`. You can do `npm install` to install your
dependencies, `npm create-react-app` to init a new react app, or `npm
rimraf folder` to remove a folder recursively.
- `npm config set default-command install` Since `npm install` is the most
common use case for npm, why not just drop the `install`? Run `npm` with
no args to install all your dependencies, or `npm react --save-peer` to
install react and save as a peer dependency.
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like it would be really confusing for a non-advanced user who uses a project with default-command set in .npmrc.

Maybe it could provide some kind of output like:

> npm foo
Running `npm run-script foo`...
"Output of npm foo here"

> npm foo --silent
"Output of npm foo here"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. Could also probably want to have a npm.log.notice('default command notice blah') so that it automatically gets placed in the debug log when stuff goes sideways.

@isaacs
Copy link
Contributor Author

isaacs commented Dec 2, 2020

Current status:

  • "Just improve help-search" seems like a pretty strong contender which removes a lot of the hazards.
  • Would really like to hear from some of the users who were asking for the default to be run-script. Would they use this?
  • Does anyone want the default to be something other than help or run-script?
  • Increasing difficulty/hazard of adding new commands is a pretty big obstacle. If we move forward to this, we basically need to decide that we are ok with whatever breakage that may cause in the future. Making any new command addition a semver-major version bump is not acceptable.

@ljharb
Copy link
Contributor

ljharb commented Dec 2, 2020

@isaacs i could see also wanting the default to be exec, but i wouldn't expect literally anything else besides help, run-script, or exec.

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Dec 2, 2020
@legodude17
Copy link

I could definitely see myself setting the default command to install, because that is the main thing I do with npm

@styfle
Copy link

styfle commented Dec 15, 2020

Would really like to hear from some of the users who were asking for the default to be run-script. Would they use this?

As a yarn user and npm user, I would definitely use this.

However, I would take it a step further and change the default from help to run-script in the next major of npm.

I didn't even realize that help was the default because I usually append --help if I need to see more info.

@oscard0m
Copy link

oscard0m commented Dec 16, 2020

I think is not explicitly specified but I think it will be covered out of the box:
When enabling this feature, npm run $command will still be working right?

I think it would be a nice to have when if we consider a .npmrc shared inside the repo to all its users (power users and beginners).

@oscard0m
Copy link

oscard0m commented Dec 16, 2020

  • Would really like to hear from some of the users who were asking for the default to be run-script. Would they use this?

@wesbos

@Jolg42
Copy link

Jolg42 commented Dec 17, 2020

Running scripts without specifying run would be amazing 💚

I know people including myself using yarn script just for that. Even if my project is managed by npm I still use yarn to run my scripts locally 😅

@wesbos
Copy link
Contributor

wesbos commented Dec 17, 2020

So if I'm reading this correctly, if we set the default-command in package.json to run-script, it will make: npm dev and npm styles run npm run dev and npm run styles assuming that npm doesn't have a command with that name?

the opt-in is interesting. I like this proposal, but I'm a bit worried that people will wonder why npm dev works sometimes and other times it's npm run dev. Further adding to the frustration that it doesn't just work. Not sure if that is warranted though..

@isaacs
Copy link
Contributor Author

isaacs commented Dec 17, 2020

So if I'm reading this correctly, if we set the default-command in package.json to run-script

s/package.json/.npmrc/, but otherwise yeah.

The much bigger obstacle, honestly, is that it means npm styles or npm dev can never be a command that we do add, without it potentially being a breaking change for anyone relying on npm styles being a shorthand for npm run styles. So, instead of adding a new command being a minor version update, it's SemVer major.

There's some support for adding a third bin called npr that'd be an alias for npm run. Or, another safe opt-in way is that you put alias npr=npm run in your ~/.bashrc file.

@wesbos
Copy link
Contributor

wesbos commented Dec 17, 2020

Makes sense, thanks! Most projects don't have an npmrc file and relying on a global config isnt ideal. I can't see people wanting to add another config file to their project to get a shortcut. As much as I want it.

It's unpopular, but I think you should just let us use anything, and if you choose to release 'npm styles' for whatever reason, just warn that there is overlap and npm takes precedence

@orta
Copy link
Contributor

orta commented Dec 18, 2020

I think yarn has this right out of the door and npm should just copy its behavior, the npm CLI should look up:

  • npm command names
  • scripts in package.json
  • ./node_modules/.bin/

When looking up a command like npm build or npm tsc.

Reasoning

Today when instructing someone show to use an npm vendored binary, you have to either recommend someone does:

npm install danger
# now edit your scripts to have "danger":"danger"
npm run danger

Which you can't automate.

or

npm install danger
npx danger

Which is a different command - but with the odd side-effect of if you get used to using npx in a project and use it elsewhere it will end up executing code you didn't include in your project. We regularly get folks telling us the instructions on the TypeScript website are wrong for using npx, even though it's not.

If you switch to yarn style:

npm install danger
npm danger

It's simpler, and doesn't need to have special cases like test or start etc.

@eps1lon
Copy link

eps1lon commented Dec 18, 2020

What I always find confusing with npm: Given 3 scripts called start, test, build. npm start works the same as npm run start, npm test works the same as npm run test, npm build throws, npm run build works.

@joshmanders
Copy link

I agree with the comments here of making it default. We're all pretty used to it already with yarn anyway. If I create a npm script called bananas and npm introduces a bananas command, that's on me, same as it is with yarn.

Only thing I'd say is if released in v7, make it default to help but allow overriding with npmrc but then in v8 make it default to run-script.

@ljharb
Copy link
Contributor

ljharb commented Dec 18, 2020

imo it would be far too disruptive to make it a default; there are far more users (and docs/tutorials) used to npm's behavior than are used to yarn's.

@orta i believe in npm 7, npx won't install things without a prompt, so that concern should go away once it's marked "latest".

@orta
Copy link
Contributor

orta commented Dec 18, 2020

I don't think that should be a problem, all of the current npm behavior is accounted for in the yarn behavior described above to my knowledge. E.g. it won't change any current working examples, and broken examples may start working.

The current behavior is much more confusing: why would it default to help when there are scripts and binaries with the name you're looking for? You'll never want npm help tsc, npm help eslint or npm help build.

This RFC looks at the answer very conservatively, but the config doesn't really make sense - run-scripts is the only useful config setting and that's only one small step. If it's per-user or per-project no-one can rely on it, and it doesn't really improve the ecosystem.

The biggest issue is around the idea of a package script being broken by having an npm command taking priority, but that's a reasonable trade-off (and one discussed above a lot)

@orta
Copy link
Contributor

orta commented Feb 24, 2021

The pnpm folks just said they'd be open to the yarn pattern of command look-ups in their CLI.

I'm open to going through the npm RFC process for adding this behavior to npm as an alternative to this RFC, then we can get consistent behavior everywhere.

@isaacs
Copy link
Contributor Author

isaacs commented Apr 20, 2021

This is not a direction we're going to go.

  • Making the addition of top-level commands a breaking change is a downside we are not willing to accept.
  • Having npm foo behave differently on a per-project basis is also undesirable.

Closing this now, as the initial issue is better addressed by #339.

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