feat: Add support for pnpm as a bundle extension#22521
Conversation
add(Library/Homebrew/bundle/extensions/pnpm.rb): ➕ add pnpm bundle extension to manage global packages add(Library/Homebrew/test/bundle/pnpm_spec.rb): ➕ add tests for pnpm extension covering listing, install, and cleanup
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds pnpm support to Homebrew Bundle by introducing a new Bundle extension and accompanying RSpec coverage for dumping, cleanup, and install flows.
Changes:
- Introduce
Homebrew::Bundle::Pnpmextension implementing package listing/install/uninstall. - Add RSpec coverage for pnpm dumping behavior (including JSON parsing) and cleanup/install logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Library/Homebrew/bundle/extensions/pnpm.rb | Adds the pnpm Bundle extension implementation (list/install/uninstall + parsing). |
| Library/Homebrew/test/bundle/pnpm_spec.rb | Adds RSpec coverage for pnpm dump/cleanup/install behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sig { params(output: String).returns(T::Array[String]) } | ||
| def parse_package_list(output) | ||
| return [] if output.blank? | ||
|
|
||
| json = JSON.parse(output) |
| sig { override.returns(T::Array[String]) } | ||
| def packages | ||
| packages = @packages | ||
| return packages if packages | ||
|
|
||
| @packages = if (pnpm = package_manager_executable) && | ||
| (!pnpm.to_s.start_with?("/") || pnpm.exist?) | ||
| with_env(package_manager_env(pnpm)) do | ||
| parse_package_list(`#{pnpm} list -g --depth=0 --json 2>/dev/null`) | ||
| end | ||
| end | ||
| return [] if @packages.nil? | ||
|
|
||
| @packages | ||
| end |
| it "returns frozen empty array when pnpm is not installed" do | ||
| allow(klass).to receive(:package_manager_installed?).and_return(false) | ||
| entries = [Homebrew::Bundle::Dsl::Entry.new(:pnpm, "vercel")] | ||
| expect(klass.cleanup_items(entries)).to eql([]) | ||
| end |
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks for the PR @cavanaug! A few high-level comments before I review the code:
Ergo, this PR. Hopefully this meets your criteria for bundle support as pnpm and I know opinions may vary, but IMO pnpm is actually a better tool than npm for several reasons.
This feature is just a stupid simple clone of the npm bundle extension for pnpm and its tests.
I don't think it makes sense to have both pnpm and npm as separate extensions. Instead, we should change the npm extension to use pnpm if available instead of npm (but still fall back to npm if it's around).
This is because they are both installing the same underlying packages from the same ecosystem.
The approach I am suggesting mirrors that which we do with e.g. vscode and the forks.
Also, please note the failing commit check here: we don't use the feat: format for commit messages.
Code Coverage OverviewLanguages: Ruby Ruby / code-coverage/simplecovThe overall coverage in the branch remains at 78%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
It’s possible to have both npm and pnpm installed and have packages installed from each. You can even have the same package from the npm registry installed by both npm and pnpm as they install in different directories. So it’s not really an either or situation it can be both. I could update and create one bundle extension that iterates over both npm and pnpm if you prefer.
Will fix. Thx for the heads up on that. |
It's possible but that's not the flow we want to support here.
We shouldn't iterate until we find one and then pick it and use it. This is what we do for other extensions. |
So Ive started using Brewfiles as part of our internal automation at work, but Im trying to move some teams internally to pnpm over npm. This is primarily due to the substantially better supply chain protections available with pnpm (and pnpm is faster, more efficient) but to really make things work effectively I needed pnpm support for brew bundles.
Ergo, this PR. Hopefully this meets your criteria for bundle support as pnpm and I know opinions may vary, but IMO pnpm is actually a better tool than npm for several reasons.
This feature is just a stupid simple clone of the npm bundle extension for pnpm and its tests.
add(Library/Homebrew/bundle/extensions/pnpm.rb): ➕ add pnpm bundle extension to manage global packages
add(Library/Homebrew/test/bundle/pnpm_spec.rb): ➕ add tests for pnpm extension covering listing, install, and cleanup
brew lgtm(style, typechecking and tests) with your changes locally?