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

Feature request: corepack run #57

Open
navarroaxel opened this issue Sep 15, 2021 · 25 comments
Open

Feature request: corepack run #57

navarroaxel opened this issue Sep 15, 2021 · 25 comments

Comments

@navarroaxel
Copy link

Add corepack run with the same behavior as [npm|yarn] run to reduce the necessity of a package manager to run user-defined scripts in the package.json. e.g. with this start script

"scripts": {
  "start": "node index.js"
}

we don't need a package manager to run:

$ corepack run start

Also, we should add the Corepack's wrappers (npm, pnpm and yarn) to the PATH automatically. Achieving the same effect like corepack enable but only on the context of the command execution to support scripts like the following:

"test": "npm run env:test && jest",

Without adding the package managers as global commands.

@arcanis
Copy link
Contributor

arcanis commented Sep 15, 2021

This isn't possible. Each package manager has different implementations of run unique to their implementation details, which Corepack couldn't replicate without becoming a package manager, which is out of scope 🙂

@navarroaxel
Copy link
Author

Maybe you can forward the args if you implement #25, then corepack run lint --fix run yarn run lint --fix or npm run lint -- --fix etc

@demurgos
Copy link

I believe there are two slightly different issues here. The choice of the package manager for the root command and for the commands inside scripts. The distinction is important because the commands in the script field are published to the registry and may be run by consumers, while the root command is only invoked in the context of the current package.

In particular, situations like the following are problematic as they require the consumer to have yarn on its system:

{
  "scripts": {
    "compile": "node-gyp ...",
    "download": "...",
    "postinstall": "yarn run download || yarn run compile"
  }
}

Yarn 2+ supports "package manager independent run" by aliasing run to yarn run so you can write:

{
  "scripts": {
    "compile": "node-gyp ...",
    "download": "...",
    "postinstall": "run download || run compile"
  }
}

This is the best solution for portable scripts calling other scripts in my opinion, I just hope this is adopted by npm, pnpm and other package managers.

corepack run may still be useful to initiate the first command. It would then pick the package manager from the one defined in package.json and would allow contributors to always run corepack run test regardless of the pm chosen by the maintainers.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2021

@demurgos the "scripts" field can not be run by consumers - it can only be run by developers of the project. Only the "bin" field are things that can be executed by consumers.

@arcanis
Copy link
Contributor

arcanis commented Sep 23, 2021

I think @demurgos' point was that postinstall scripts find it difficult to execute scripts in a portable way across package managers (in a way that whatever package manager is used to install the package will also be called to run the scripts). The two options are currently to either hardcode yarn run / npm run, or to write an awkward reference to $npm_execpath.

Yarn supports calling run <script name> within scripts (ie without expliciting the package manager); if npm/pnpm supported that it would solve this particular problem.

As for corepack run, I'm not sure how useful it would be in practice ... you typically have to install your deps to run scripts successfully, and I suspect muscular memory would make corepack run a rarely use command 🤔

@demurgos
Copy link

demurgos commented Sep 24, 2021

@demurgos the "scripts" field can not be run by consumers - it can only be run by developers of the project. Only the "bin" field are things that can be executed by consumers.

Some of the the scripts fields are definitely executed by package consumers (so you don't control their environment). These are mostly lifecycle scripts. install/postinstall are probably the best example because they are supported by all package managers: when a consumer installs a package, these scripts are always executed. I believe there are other fields such as prepare when using git:// dependencies, or the deprecated prepublish.

Even if I find run <script name> to be a cleaner solution to handle these cases, it requires support by the major package managers to be truly portable. corepack run <script name> could provide a portable solution without requiring extra work from package managers.

@ljharb
Copy link
Member

ljharb commented Sep 24, 2021

Ha, good point, i wasn’t thinking about lifecycle scripts.

@donferi
Copy link

donferi commented Nov 1, 2021

I'm not sure how useful it would be in practice ... you typically have to install your deps to run scripts successfully, and I suspect muscular memory would make corepack run a rarely use command 🤔

I believe there are some legitimate usecases. Muscle memory works when you use the same package manager in all the projects you are working on but that is not always the case.

https://github.com/antfu/ni Seems to be somewhat popular to address some of this, not the best data point but it shows a bit of community thirst for something like this.

Would also help when writing a sharable CI config in orgs where they use multiple package managers. Simplifying running commands / installing deps / etc.

@styfle
Copy link
Member

styfle commented Jun 10, 2022

Also checkout https://github.com/egoist/dum which solves most of these use cases.

I would like a feature like this, although I'm not sure this is the responsibility of corepack. Or rather, I wouldn't want it exposed as a public command through corepack cli.

Ideally, this feature would be exposed like node --run <scriptname>.

@arcanis
Copy link
Contributor

arcanis commented Jun 10, 2022

Ideally, this feature would be exposed like node --run <scriptname>

Running scripts depend on knowledge unique to each package manager, so it couldn't be in Node (at best it perhaps could be in Corepack since it could then delegate to the right package manager, but even that could lead into conflict problems).

@styfle
Copy link
Member

styfle commented Jun 10, 2022

What I mean is that I don't think the user should be interacting with the corepack cli (it could still be implemented in corepack under the hood though). For example, if corepack is enabled by default (#104), then users won't really ever use corepack cli so it would feel strange to suddenly introduce it to run scripts.

@aduh95
Copy link
Contributor

aduh95 commented Jun 10, 2022

What I mean is that I don't think the user should be interacting with the corepack cli (it could still be implemented in corepack under the hood though). For example, if corepack is enabled by default (#104), then users won't really ever use corepack cli so it would feel strange to suddenly introduce it to run scripts.

Adding a feature to corepack CLI doesn't force anyone to use it though, it may make that feature harder to discover, but that doesn't seem to be a good enough reason for rejecting it on that basis alone.

FWIW, it happened to me more than once to try to run e.g. corepack add … instead of corepack yarn add …, and I suppose that Corepack could have made the smart thing to redirect any unknown command to whatever package manager defined in the package.json. Yarn does have something similar, if it receives a command it doesn't know, it guesses if that's a script defined in package.json or a bin in node_modules. @arcanis has this pattern ever been problematic for Yarn?

@arcanis
Copy link
Contributor

arcanis commented Jun 10, 2022

@arcanis has this pattern ever been problematic for Yarn?

The situation is different - Yarn is a single program, so even if there's a conflict between its commands and the user scripts, those problem tends to be easily spotted (commands and scripts tend to do very different things).

In Corepack, we are abstracting multiple package managers, each with their own different behaviours (for example yarn run build:foo will run the build:foo script regardless of the workspace that declares it, whereas npm run build:foo will run it exclusively in the current workspace). While the semantics are often similar, the details aren't, and I feel like the subtle differences in behaviour could be extremely confusing for our users and painful for us as maintainers.

For this reason, I agree with @styfle that the corepack commands shouldn't be used in place of the individual package manager commands (which echoes my comment in #100 (comment)), and rather than corepack yarn add ... you should just call yarn add ... (or npm add ... or pnpm add ...). It removes ambiguity, and lets each package manager own their full UX.

Adding a feature to corepack CLI doesn't force anyone to use it though

I don't agree with this - the JS community is large, and many many many people would misinterpret the very existence of such an optional feature in Node as meaning "it's official, you don't need to specify the package manager name anymore" - then they would get pissed when it doesn't work as they expect from one project to the other, or because it doesn't work for all commands, or [...] - eventually blaming it on either Corepack or package manager authors.

@aduh95
Copy link
Contributor

aduh95 commented Jun 10, 2022

Adding a feature to corepack CLI doesn't force anyone to use it though

I don't agree with this - the JS community is large, and many many many people would misinterpret the very existence of such an optional feature as meaning "you don't need to specify the package manager name" - then get pissed when it doesn't work as they expect from one project to the other, or because it doesn't work for all commands, or [...] - eventually blaming it on either Corepack or package manager authors.

To clarify, I agree that "we don't see a way to make it work without breaking users expectations" is a perfectly valid reason for not implementing it. I just wanted to point out that "I don't think the user should be interacting with the corepack cli" is not imo, because folks who don't want to use corepack cli won't be forced to use it whether or not we add features to it.

@x11x
Copy link
Contributor

x11x commented Jul 5, 2022

Just wanted to mention another package https://github.com/BendingBender/yarpm as a solution to the package.json scripts issue.

I liked that the readme for yarpm makes it clear that it is not trying to be a package manager abstraction layer and that package authors must make sure that their scripts are compatible with all package managers if they want to use yarpm in their package.json scripts. This seems important for package authors to know, that you don't just get compatibility with all package managers for free (to arcanis' point above).

@styfle
Copy link
Member

styfle commented Oct 13, 2022

It sounds like the path forward here is to create a npm RFC for the run shorthand that yarn supports (mentioned above).

Are there any yarn docs that the npm team can reference to make sure it works the same way if/when they implement it?

@ljharb
Copy link
Member

ljharb commented Oct 14, 2022

Adding a new top-level binary is likely to be a nonstarter; npx is already regretted.

@arcanis
Copy link
Contributor

arcanis commented Oct 14, 2022

I should add a note about it to our documentation, for sure.

Adding a new top-level binary is likely to be a nonstarter; npx is already regretted.

This isn't a top-level binary in the same way you may think about it.

It's only available inside scripts commands, so it doesn't pollute the global user namespace (in terms of implementation, I suppose npm would add a run bin to node_modules/.bin).

@ljharb
Copy link
Member

ljharb commented Oct 14, 2022

What would be the point of that? run as an alias to npm run?

@arcanis
Copy link
Contributor

arcanis commented Oct 14, 2022

Make install scripts able to cross-refer to other scripts without having to hardcode package manager names (or use $npm_execpath, which practically noone does).

@aduh95
Copy link
Contributor

aduh95 commented Oct 14, 2022

(or use $npm_execpath, which practically noone does).

TIL, that would indeed fix the issue! I suspect most package.json authors are not aware of this.

@styfle
Copy link
Member

styfle commented Oct 14, 2022

I must have missed that above!

I tried $npm_execpath with the example above and it works today 🤩

{
  "scripts": {
    "compile": "echo 'compile done'",
    "download": "echo 'download done'",
    "postinstall": "$npm_execpath run download && $npm_execpath run compile"
  }
}
  • npm run postinstall - ✅ works
  • yarn run postinstall - ✅ works
  • pnpm run postinstall - ✅ works

@styfle
Copy link
Member

styfle commented Oct 14, 2022

Oh no, I just realized that $npm_execpath doesn't work on Windows 🤦

That explains why no one would use it for postinstall.

@arcanis
Copy link
Contributor

arcanis commented Oct 14, 2022

Although it doesn't solve the underlying portability problem, it actually works for Yarn Modern even on Windows.

@styfle
Copy link
Member

styfle commented Apr 4, 2024

Looks like this might be solved with node run or perhaps node --run

mergify bot added a commit to Agoric/agoric-sdk that referenced this issue Apr 23, 2024
closes: #8288

## Description

ui-kit was recently bumped to Yarn 4 and @samsiegart hit a snag that
required patching :
Agoric/ui-kit@464f61d

This removes `yarn` dependence in [NPM lifecycle
scripts](https://docs.npmjs.com/cli/v10/using-npm/scripts#life-cycle-operation-order).
It does so by using `npm run`, which should be available everywhere. In
Node 22 we'll [have `node
--run`](nodejs/corepack#57 (comment))
available.

### Security Considerations

none
### Scaling Considerations

none

### Documentation Considerations

none
### Testing Considerations

working in
Agoric/ui-kit@464f61d

### Upgrade Considerations

none
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

No branches or pull requests

8 participants