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

pnpm support #730

Merged
merged 25 commits into from Feb 26, 2024
Merged

pnpm support #730

merged 25 commits into from Feb 26, 2024

Conversation

mmkal
Copy link
Collaborator

@mmkal mmkal commented Jan 31, 2024

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

@mmkal mmkal marked this pull request as draft January 31, 2024 18:36
@mmkal
Copy link
Collaborator Author

mmkal commented Jan 31, 2024

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 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
source/ui.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

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

@sindresorhus
Copy link
Owner

// @transitive-bullshit @mifi

@mifi
Copy link
Sponsor Contributor

mifi commented Feb 6, 2024

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

@mmkal mmkal marked this pull request as ready for review February 8, 2024 23:31
@mmkal
Copy link
Collaborator Author

mmkal commented Feb 8, 2024

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 Outdated Show resolved Hide resolved
source/ui.js Outdated Show resolved Hide resolved
source/util.js Outdated Show resolved Hide resolved
@@ -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';
Copy link
Collaborator

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

@tommy-mitchell
Copy link
Collaborator

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.

Copy link
Sponsor Contributor

@mifi mifi left a 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

@tommy-mitchell
Copy link
Collaborator

This worked great with Yarn v1.

@mmkal
Copy link
Collaborator Author

mmkal commented Feb 22, 2024

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
@mmkal
Copy link
Collaborator Author

mmkal commented Feb 23, 2024

@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.

readme.md Outdated Show resolved Hide resolved
Copy link
Sponsor Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes works fine

source/package-manager/types.d.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 4b3b599 into sindresorhus:main Feb 26, 2024
1 check passed
@sindresorhus
Copy link
Owner

@mmkal Looks good. Thanks for working on this 👍

Vylpes pushed a commit to Vylpes/vylbot-app that referenced this pull request Apr 10, 2024
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 ([#&#8203;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 ([#&#8203;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 ([#&#8203;730](sindresorhus/np#730))  [`4b3b599`](sindresorhus/np@4b3b599)
    -   The functionality is replaced by `--package-manager`. See below.

##### Improvements

-   Add `--package-manager` flag ([#&#8203;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 ([#&#8203;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>
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

Successfully merging this pull request may close these issues.

Support pnpm in 2024?
4 participants