-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
Add support for pnpm (don't merge) #667
Add support for pnpm (don't merge) #667
Conversation
@@ -0,0 +1,11 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not having this here tripped me up a lot in the beginning, since prettier is a pretty common default config
> This is a fork of [np](https:/sindresorhus/np) which adds support for [pnpm](https://pnpm.io). The maintainers of np [understandably don't want to support every package manager](https:/sindresorhus/np/issues/251#issuecomment-400717095), so that's why this fork exists. | ||
> | ||
> Aside from adding support for `pnpm`, this fork is equivalent to the upstream version. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this header notice would need to be removed if we decided to not go with the fork
|
||
const exec = (cmd, args) => { | ||
// Use `Observable` support if merged https:/sindresorhus/execa/pull/26 | ||
const cp = execa(cmd, args); | ||
|
||
if (!cp.stdout || !cp.stdout.pipe) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept getting errors when running the ava
unit tests about cp.stdout.pipe
and cp.stderr.pipe
not existing.
Looking at the test impl using sinon.sub
, I don't understand how this code was stubbing and working correctly previously, so hopefully someone can tell me what I'm missing 😄
module.exports = async (input = 'patch', options) => { | ||
if (!hasYarn() && options.yarn) { | ||
if (options.yarn && !hasYarn()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched the order here so we don't run the extra FS functions is yarn
and pnpm
aren't used.
@transitive-bullshit I don't have objections to keep a fork in the pnpm org if you are willing to maintain it. |
I'm happy to add support for ppm here if someone can commit to maintain it. |
This would be epic 🔥 |
Would gladly award bug bounty or sponsor you on GitHub sponsors if you can help push this to the finish line and get it baked into |
This PR looks good to me. I'm willing to maintain pnpm support in np if this gets merged. |
@sindresorhus @wmertens I will also be using it, and therefore will also help to keep it running… |
@ the people on this thread: I missed that this exists, so I made an another: #730 That one is a bigger change, which refactors the package and adds a The diff is more significant, but the end result is that this repository itself is easier to grok, IMO. And it actually reduces the overall code. I've volunteered to maintain the pnpm config, but if there are existing maintainers on this thread who would like to take a look, feedback is welcome. There's a fuller description in the PR body. Note: since there's no build step, you can also try it out directly by putting |
Closing in favor of #730 |
This PR adds support for the
pnpm
package manager, which has been steadily growing in popularity.The maintainers don't want to add support for every package manager (see #251 (comment)), which is totally understandable — and therefore I don't expect this PR to be merged.
I'm just opening the PR here for visibility and to have a chance to gather feedback.
Note that I haven't added appropriate top-level unit tests for
pnpm
yet.cc @zkochan depending on what the
np
maintainers decide, would it possibly make sense to move this fork under thepnpm
organization? I haven't published it to NPM yet.