sindresorhus / np

A better `npm publish`
MIT License
7.56k stars 299 forks source link

pnpm support #730

Closed mmkal closed 9 months ago

mmkal commented 9 months ago

Fixes #729

~This is starting out as a draft, because it ended up being a fairly big change. If it's unwanted, I can scrap this and do a smaller change that adds pnpm support similarly to yarn, but it was already very messy, so I thought I'd clean up.~

In the end by adding pnpm support there are net ~50 lines of code fewer, and one dependency dropped.

User-facing changes:

Outside of the above, I tried as hard as I could do make the logic identical as far as the user is concerned.

Implementation:

Potential simplifications:

Other stuff:

I use np regularly and would happy to join as a maintainer if that'd be helpful.

Testing:

FYI @sindresorhus @zkochan

mmkal commented 9 months ago

Ah, I should have checked pull requests as well as issues: https://github.com/sindresorhus/np/pull/667

That one is the smaller, backwards compatible, but messier change, adding --pnpm/--no-pnpm args etc.

I guess that PR going quiet isn't a good sign in terms of maintenance, but if this change would be accepted, I'd be happy to maintain it going forward.

sindresorhus commented 9 months ago

This looks like a good start to me. I agree with the general direction. 👍

sindresorhus commented 9 months ago

// @transitive-bullshit @mifi

mifi commented 9 months ago

I like these simplifications, and can help to test the yarn berry functionality once ready

mmkal commented 9 months ago

Tests now passing for me locally (with the 1 known failure).

Edit: oops, forgot to run xo. Will do soon, but this is ready for review if you're willing to ignore missing semicolon etc.

sindresorhus commented 9 months ago

np could be stricter about package managers. We could say you can only resolve a package manager based on the packageManager field, no looking at lock files. I think that would simplify things, and still make it easy for anyone who wanted to override with a specific behaviour to get what they want. But that would be a more significant breaking change, so I didn't do it for now.

Open an issue about this. I think we could do this in a couple of years when the packageManager field is more commonly used.

sindresorhus commented 9 months ago

Tried this out for a bit now. This seems to work well for me.

sindresorhus commented 9 months ago

@mifi I believe this is ready for review.

tommy-mitchell commented 9 months ago

np could be stricter about package managers. We could say you can only resolve a package manager based on the packageManager field, no looking at lock files.

I think automatically determining the package manager via the lockfile makes np that much nicer to use - it "just works."

I think we could do this in a couple of years when the packageManager field is more commonly used.

That's also fair.

tommy-mitchell commented 9 months ago

Sorry for taking so long to review, this all looks really good to me as well. Thanks for putting this together. I use Yarn so I can give it a try for publishing with it.

tommy-mitchell commented 9 months ago

This worked great with Yarn v1.

mmkal commented 9 months ago

Thanks for the feedback - will try to incorporate it this week.

mmkal commented 9 months ago

@tommy-mitchell pushed your suggestions minus the () => enabled -> enabled one.

@mifi I squashed in https://github.com/mmkal/np/pull/1 - thank you v much. I like the installCommandNoLockfile better than what I had before. I made some changes in https://github.com/sindresorhus/np/pull/730/commits/3163835537d8b20290c0a77e80f422c42d153da6 though:

@mifi would you be able to test on yarn berry one more time 😬? I just successfully did with pnpm again.

sindresorhus commented 9 months ago

@mmkal Looks good. Thanks for working on this 👍