pnpm support by mmkal · Pull Request #730 · sindresorhus/np

mmkal · 2024-01-31T18:35:54Z

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:

new option --package-manager - this acts like the packageManager field in package.json. np will default to reading package.json, and look for lockfiles to determine the best package manager as a last resort.
removed option --yarn (and --no-yarn). This is a breaking change. The functionality is replaced by --package-manager (if you want yarn, instead of --yarn set "packageManager": "yarn@1.7.0" in package.json or --package-manager yarn@1.7.0 via CLI. If you don't want yarn, set "packageManager": "npm@9.0.0" or whatever in package.json)
minor/I think nobody would notice: np now does npm run test/yarn run test/pnpm run test instead of npm test etc.

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

Implementation:

add a package-manager subfolder in source/
this defines a type for a package manager config - the config replaces all the scary ternary logic that was everywhere with npm vs yarn vs yarn berry
each config defines its main CLI command, how to install, how to bump a version, how to lookup the registry url, how to publish, and its expected lockfiles. There are configs defined for npm, pnpm, yarn and yarn-berry

Potential simplifications:

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.

Other stuff:

I added JSDoc typescript types on a few internal methods, mostly so I could get autocomplete which I have become unhealthily dependent on over the years. I can remove if people find it ugly, but I think it's helpful and may help port to typescript some day, if that's deemed a good idea.
For the sake of inferring types, I broke up some parts of cli-implementation.js and exported functions. It doesn't affect the API surface of this package but is potentially a bit... weird.
I saw some pieces that looked like they already wouldn't be working with yarn berry, will comment inline
tests are passing with minor updates ~~tests were failing for me on main, so I haven't really touched them beyond removing tests for deleted code~~
I updated docs, ~~but not tests. I didn't want to touch tests until I got some direction on whether maintainers would be willing to accept a PR this big. I did notice they are broken for me on main, though.~~

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

Testing:

I successfully published a real package using npm with this to make sure it wasn't regressing existing functionality
I also published a package using pnpm, although I saw an issue with the bumped package.json not being committed. This happened with np v9.2.0 too, though, so I think it's a separate issue
I haven't tested yarn because I don't use it. Would be great to get some help with that.
Failing that, or even with that, IMO this should be published as a prerelease of a major version bump, because of the changed CLI arg, the fairly big diff, and how much mocking is involved in the tests

FYI @sindresorhus @zkochan

source/npm/publish.js

mmkal · 2024-01-31T20:18:25Z

Ah, I should have checked pull requests as well as issues: #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.

readme.md

source/cli-implementation.js

source/index.js

source/ui.js

source/package-manager/configs.js

sindresorhus · 2024-02-06T10:15:20Z

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

sindresorhus · 2024-02-06T10:16:35Z

// @transitive-bullshit @mifi

mifi · 2024-02-06T11:36:07Z

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

mmkal · 2024-02-08T23:32:41Z

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.

source/index.js

source/ui.js

source/util.js

tommy-mitchell · 2024-02-15T05:26:15Z

test/tasks/prerequisite-tasks.js

 import {SilentRenderer} from '../_helpers/listr-renderer.js';
 import {_createFixture} from '../_helpers/stub-execa.js';
 import {run, assertTaskFailed, assertTaskDisabled} from '../_helpers/listr.js';
+import {npmConfig, yarnConfig} from '../../source/package-manager/configs.js';


This import should be moved up as well

tommy-mitchell · 2024-02-15T05:33:42Z

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.

mifi

thanks for finishing this. I've done some testing on yarn berry but it was broken so I created a new branch (and PR) to fix it. feel free to merge it into this branch if you think everything looks ok: mmkal#1

tommy-mitchell · 2024-02-16T04:48:44Z

This worked great with Yarn v1.

mmkal · 2024-02-22T03:06:48Z

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

commit 92903bc Author: Mikael Finstad <finstaden@gmail.com> Date: Fri Feb 16 11:44:18 2024 +0800 Update source/package-manager/types.d.ts Co-authored-by: Tommy <tmitchell7@uh.edu> commit 9858c3d Author: Mikael Finstad <finstaden@gmail.com> Date: Thu Feb 15 15:40:44 2024 +0800 fix bugs discovered when testing yarn berry

source/index.js

mmkal · 2024-02-23T21:15:28Z

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

@mifi I squashed in mmkal#1 - thank you v much. I like the installCommandNoLockfile better than what I had before. I made some changes in 3163835 though:

instead of having a publishCli and some "prefix args", I made publishCommand a function which returns a command, similar to versionCommand.
- That makes it less awkward/rigid about what it can do. If yarn v3 remaps every single publish cli arg, it will be possible to handle that by doing publishCommand: args => ['yarn', remapAllTheArgsWithArbitraryComplexRules(args)].
- Similarly if future supported package managers like bun, deno, nuget, etc. etc. have totally different publishing interfaces, it could still be supported (although in that case, it may make more sense to just expose the PackageManagerConfig and let downstream users define their own configs or something).

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

source/package-manager/index.js

readme.md

mifi

yes works fine

source/package-manager/types.d.ts

sindresorhus · 2024-02-26T12:55:55Z

@mmkal Looks good. Thanks for working on this 👍

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [np](https://github.com/sindresorhus/np) | devDependencies | major | [`^9.0.0` -> `^10.0.0`](https://renovatebot.com/diffs/npm/np/9.2.0/10.0.2) | --- ### Release Notes <details> <summary>sindresorhus/np (np)</summary> ### [`v10.0.2`](https://github.com/sindresorhus/np/releases/tag/v10.0.2) [Compare Source](sindresorhus/np@v10.0.1...v10.0.2) - Use npm for tagging versions when pnpm is the chosen package manager ([#739](sindresorhus/np#739)) [`770418f`](sindresorhus/np@770418f) ### [`v10.0.1`](https://github.com/sindresorhus/np/releases/tag/v10.0.1) [Compare Source](sindresorhus/np@v10.0.0...v10.0.1) - Fix compatibility with npm 10 ([#737](sindresorhus/np#737)) [`9fdebd5`](sindresorhus/np@9fdebd5) ### [`v10.0.0`](https://github.com/sindresorhus/np/releases/tag/v10.0.0) [Compare Source](sindresorhus/np@v9.2.0...v10.0.0) ##### Breaking - Remove the `--yarn` flag ([#730](sindresorhus/np#730)) [`4b3b599`](sindresorhus/np@4b3b599) - The functionality is replaced by `--package-manager`. See below. ##### Improvements - Add `--package-manager` flag ([#730](sindresorhus/np#730)) [`4b3b599`](sindresorhus/np@4b3b599) - This acts like the [`packageManager` field](https://nodejs.org/api/packages.html#packagemanager) in package.json. `np` will default to reading package.json, and look for lockfiles to determine the best package manager as a last resort. - Add pnpm support ([#730](sindresorhus/np#730)) [`4b3b599`](sindresorhus/np@4b3b599) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/vylbot-app/pulls/415 Co-authored-by: Renovate Bot <renovate@vylpes.com> Co-committed-by: Renovate Bot <renovate@vylpes.com>

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [np](https://github.com/sindresorhus/np) | devDependencies | major | [`^9.0.0` -> `^10.0.0`](https://renovatebot.com/diffs/npm/np/9.2.0/10.0.5) | --- ### Release Notes <details> <summary>sindresorhus/np (np)</summary> ### [`v10.0.5`](https://github.com/sindresorhus/np/releases/tag/v10.0.5) [Compare Source](sindresorhus/np@v10.0.4...v10.0.5) - Fix npm 10 compatibility for real ([#744](sindresorhus/np#744)) [`34409be`](sindresorhus/np@34409be) ### [`v10.0.4`](https://github.com/sindresorhus/np/releases/tag/v10.0.4) [Compare Source](sindresorhus/np@v10.0.3...v10.0.4) - Fix compatibility with npm 10 ([#743](sindresorhus/np#743)) [`4caa295`](sindresorhus/np@4caa295) ### [`v10.0.3`](https://github.com/sindresorhus/np/releases/tag/v10.0.3) [Compare Source](sindresorhus/np@v10.0.2...v10.0.3) - Fix publish OTP for Yarn Berry ([#741](sindresorhus/np#741)) [`02f60c7`](sindresorhus/np@02f60c7) ### [`v10.0.2`](https://github.com/sindresorhus/np/releases/tag/v10.0.2) [Compare Source](sindresorhus/np@v10.0.1...v10.0.2) - Use npm for tagging versions when pnpm is the chosen package manager ([#739](sindresorhus/np#739)) [`770418f`](sindresorhus/np@770418f) ### [`v10.0.1`](https://github.com/sindresorhus/np/releases/tag/v10.0.1) [Compare Source](sindresorhus/np@v10.0.0...v10.0.1) - Fix compatibility with npm 10 ([#737](sindresorhus/np#737)) [`9fdebd5`](sindresorhus/np@9fdebd5) ### [`v10.0.0`](https://github.com/sindresorhus/np/releases/tag/v10.0.0) [Compare Source](sindresorhus/np@v9.2.0...v10.0.0) ##### Breaking - Remove the `--yarn` flag ([#730](sindresorhus/np#730)) [`4b3b599`](sindresorhus/np@4b3b599) - The functionality is replaced by `--package-manager`. See below. ##### Improvements - Add `--package-manager` flag ([#730](sindresorhus/np#730)) [`4b3b599`](sindresorhus/np@4b3b599) - This acts like the [`packageManager` field](https://nodejs.org/api/packages.html#packagemanager) in package.json. `np` will default to reading package.json, and look for lockfiles to determine the best package manager as a last resort. - Add pnpm support ([#730](sindresorhus/np#730)) [`4b3b599`](sindresorhus/np@4b3b599) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.vylpes.xyz/RabbitLabs/random-bunny/pulls/159 Co-authored-by: Renovate Bot <renovate@vylpes.com> Co-committed-by: Renovate Bot <renovate@vylpes.com>

mmkal added 6 commits January 30, 2024 23:46

support pnpm

45902ae

getTagVersionPrefix

a56a460

don't assume npm

a100281

exec(...) -> from(execa(...))

c577293

tagVersionPrefix commands + jsdoc

9a21404

more yarn stragglers

6a58244

mmkal marked this pull request as draft January 31, 2024 18:36

mmkal commented Jan 31, 2024

View reviewed changes

source/npm/publish.js Show resolved Hide resolved

mmkal added 3 commits January 31, 2024 13:39

xo --fix

2510e70

a

b56ed7b

rm unused import

3f2fbf5

mmkal mentioned this pull request Jan 31, 2024

Add support for pnpm (don't merge) #667

Closed

mmkal added 3 commits February 1, 2024 20:45

Simplify lockfile update check

61b6cf2

match old args

836f0ff

xo --fix

10243c5

mmkal commented Feb 2, 2024

View reviewed changes

readme.md Show resolved Hide resolved

source/cli-implementation.js Outdated Show resolved Hide resolved

source/cli-implementation.js Show resolved Hide resolved

source/index.js Show resolved Hide resolved

source/index.js Outdated Show resolved Hide resolved

mmkal mentioned this pull request Feb 3, 2024

add types to package.json sindresorhus/meow#253

Closed

sindresorhus reviewed Feb 6, 2024

View reviewed changes

source/ui.js Outdated Show resolved Hide resolved

sindresorhus reviewed Feb 6, 2024

View reviewed changes

source/package-manager/configs.js Outdated Show resolved Hide resolved

sindresorhus requested a review from tommy-mitchell February 6, 2024 10:17

mmkal added 3 commits February 8, 2024 16:02

nickname -> id

e8bc4b9

start making tests pass

0cab44f

shooting stars

b866b22

mmkal marked this pull request as ready for review February 8, 2024 23:31

xo --fix

b0eb552