-
-
Notifications
You must be signed in to change notification settings - Fork 299
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
pnpm support #730
pnpm support #730
Conversation
Ah, I should have checked pull requests as well as issues: #667 That one is the smaller, backwards compatible, but messier change, adding 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. |
This looks like a good start to me. I agree with the general direction. 👍 |
I like these simplifications, and can help to test the yarn berry functionality once ready |
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. |
test/tasks/prerequisite-tasks.js
Outdated
@@ -5,6 +5,7 @@ import {npPkg} from '../../source/util.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'; |
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 import should be moved up as well
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. |
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.
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
This worked great with Yarn v1. |
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
@tommy-mitchell pushed your suggestions minus the @mifi I squashed in mmkal#1 - thank you v much. I like the
@mifi would you be able to test on yarn berry one more time 😬? I just successfully did with pnpm again. |
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.
yes works fine
@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. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4wLjAiLCJ1cGRhdGVkSW5WZXIiOiIzNy4wLjAiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=--> 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. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4wLjAiLCJ1cGRhdGVkSW5WZXIiOiIzNy4wLjAiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=--> 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>
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:
--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.--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)np
now doesnpm run test
/yarn run test
/pnpm run test
instead ofnpm 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:
package-manager
subfolder insource/
Potential simplifications:
np
could be stricter about package managers. We could say you can only resolve a package manager based on thepackageManager
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:
cli-implementation.js
and exported functions. It doesn't affect the API surface of this package but is potentially a bit... weird.tests were failing for me on main, so I haven't really touched them beyond removing tests for deleted codebut 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 onmain
, though.I use
np
regularly and would happy to join as a maintainer if that'd be helpful.Testing:
npm
with this to make sure it wasn't regressing existing functionalitypnpm
, 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 issueFYI @sindresorhus @zkochan