Repository navigation
Fix vendored pnpm deployments calling pnpm install instead of pnpm rebuild - #964
Merged
Merged
Conversation
…build When a pnpm project has a pre-populated node_modules/ (vendored deployment), the buildpack should skip the install phase and only rebuild native addons. Two bugs prevented this: 1. The BuildDependencies switch matched `case s.UsePNPM` before `case s.IsVendored`, so vendored pnpm projects always ran `pnpm install --no-frozen-lockfile`. 2. The vendored case hardcoded `NPM.Rebuild()`, so even with corrected ordering a pnpm vendored project would have run npm instead of pnpm. Fix by splitting `case s.UsePNPM` into two cases (vendored and non-vendored), adding `Rebuild()` to the PNPM interface, and implementing it in pnpm.go mirroring the npm rebuild approach. Fixes #957
- supply_test.go: add MockPNPM, wire it into the Supplier, and add three BuildDependencies cases for pnpm: non-vendored calls PNPM.Build, vendored calls PNPM.Rebuild, and vendored does NOT fall through to NPM. - supply/mocks_test.go: add MockPNPM (Build + Rebuild) matching the updated PNPM interface. - pnpm/pnpm_test.go + pnpm_suite_test.go: new test suite covering Rebuild (verifies args, dir, NODE_HOME env, error propagation) and Build (online and offline modes).
5 of 6 tasks
Contributor
🔗 Related: PR #965 - Yarn Vendored FixDuring code review of this PR, I identified that Yarn has the identical bug (as mentioned in issue #957). I've opened PR #965 which applies the same fix pattern to Yarn:
Summary of Fixes
Both PRs follow the same implementation pattern and should be reviewed/merged together to ensure all three package managers have consistent vendored deployment behavior. |
tnikolova82
approved these changes
Oct 12, 2026
bvincent
reviewed
Oct 12, 2026
bvincent
left a comment
bvincent
left a comment
There was a problem hiding this comment.
Thanks for taking care of the issue. Some remarks, let me know if any more informations is needed.
|
|
||
| func (p *PNPM) Rebuild(buildDir string) error { | ||
| p.Log.Info("Rebuilding any native modules (pnpm)") | ||
| cmd := exec.Command("pnpm", "rebuild") |
There was a problem hiding this comment.
As pnpm workspace is supported, we should maybe use recursive option in order to try to rebuild all packaged modules.
|
|
||
| It("does NOT call npm when pnpm is vendored", func() { | ||
| supplier.IsVendored = true | ||
| mockPNPM.EXPECT().Rebuild(buildDir).Return(nil) |
There was a problem hiding this comment.
Test not fit the it description, he no test npm calls.
Contributor
Author
There was a problem hiding this comment.
fixed the test description
- pnpm.go: add --recursive to pnpm rebuild to handle workspace projects where modules are spread across packages (per pnpm.io/cli/rebuild) - supply_test.go: make the npm-not-called assertion explicit using Times(0) rather than relying on implicit gomock strict-mode behaviour; rename test description to match what it actually asserts - pnpm_test.go: update expected args to include --recursive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #957
Vendored pnpm deployments (projects with a pre-populated
node_modules/) were incorrectly runningpnpm install --no-frozen-lockfileinstead ofpnpm rebuild. There were two bugs inBuildDependencies():Bug 1 — Switch case ordering
case s.UsePNPMwas evaluated beforecase s.IsVendored. When both flags are set (i.e.pnpm-lock.yamlexists andnode_modules/is pre-populated), the pnpm install path always won.Bug 2 — Vendored case hardcoded NPM
Even with correct ordering,
case s.IsVendoredcalleds.NPM.Rebuild()unconditionally, meaning a vendored pnpm project would have invoked npm instead of pnpm.Changes
src/nodejs/pnpm/pnpm.go: addRebuild(buildDir string) errormethod that runspnpm rebuildwithNODE_HOMEset, mirroring the npm implementation.src/nodejs/supply/supply.go: addRebuild(string) errorto thePNPMinterface; splitcase s.UsePNPMinto two cases —s.UsePNPM && s.IsVendored(callsPNPM.Rebuild) ands.UsePNPM(callsPNPM.Build) — so vendored detection takes priority over the package manager install path.Test plan
pnpm-lock.yaml+node_modules/present) callsPNPM.Rebuild, notPNPM.BuildPNPM.Buildas beforeNPM.Rebuild(no regression)node_modules/andpnpm-lock.yaml; confirmpnpm rebuildis called and the app stages successfully