From e96746ce5862337b816d0ac56741edf6354a04cc Mon Sep 17 00:00:00 2001 From: i343759 Date: Mon, 12 Oct 2026 14:53:59 +0300 Subject: [PATCH] Fix vendored Yarn deployments calling yarn install instead of yarn rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Following the same pattern as PR #964 (pnpm vendored fix), Yarn projects with pre-populated `node_modules/` (vendored deployments) incorrectly run `yarn install` instead of `yarn rebuild`. There are two distinct bugs in `BuildDependencies()`: ### Bug 1 — `case s.UseYarn` has priority over `case s.IsVendored` ```go switch { case s.UseYarn: // Matched first, Yarn.Build() runs s.Yarn.Build(...) case s.UsePNPM: s.PNPM.Build(...) case s.IsVendored: // Never reached when yarn.lock is present s.NPM.Rebuild(...) ... } ``` When both flags are set (yarn.lock exists AND node_modules/ exists), `Yarn.Build()` runs `yarn install`, ignoring the pre-populated `node_modules/`. ### Bug 2 — `case s.IsVendored` hardcodes `NPM.Rebuild()` regardless of package manager Even with correct ordering, `case s.IsVendored` always calls `s.NPM.Rebuild()`. A vendored Yarn project should call `Yarn.Rebuild()`. ## Solution 1. **Add `Rebuild(buildDir string) error` to the `Yarn` interface** - Mirrors the npm implementation - Mirrors the pnpm fix from PR #964 2. **Implement `Yarn.Rebuild()` in `src/nodejs/yarn/yarn.go`** - Runs `yarn rebuild` with `NODE_HOME` set via `npm_config_nodedir` - Ensures native modules compile for the CF stack 3. **Add `case s.UseYarn && s.IsVendored` before `case s.UseYarn`** - Vendored detection now takes priority over the package manager install path - Calls `Yarn.Rebuild()` instead of `Yarn.Build()` ## Changes - **`src/nodejs/yarn/yarn.go`**: Add `Rebuild()` method that runs `yarn rebuild` - **`src/nodejs/supply/supply.go`**: - Add `Rebuild(string) error` to the `Yarn` interface - Add `case s.UseYarn && s.IsVendored` case before `case s.UseYarn` - **`src/nodejs/yarn/yarn_test.go`**: Add unit tests for `Rebuild()` - **`src/nodejs/supply/supply_test.go`**: Add vendored yarn test cases - **`src/nodejs/supply/mocks_test.go`**: Add `Rebuild()` mock method ## Test Coverage - [x] Unit test: vendored Yarn project calls `Yarn.Rebuild`, not `Yarn.Build` - [x] Unit test: non-vendored Yarn project calls `Yarn.Build` as before - [x] Unit test: vendored npm project still calls `NPM.Rebuild` (no regression) - [x] Unit test: `Yarn.Rebuild()` runs `yarn rebuild` with NODE_HOME set - [ ] Integration test: push CF app with pre-populated `node_modules/` and `yarn.lock` ## Use Case Teams using `yarn install --production` or workspace vendoring to produce a pre-populated `node_modules/` directory before pushing. The buildpack should detect this and skip the install phase, running `yarn rebuild` instead to recompile any native addons for the CF stack. ## Related - Issue #957: Original bug report for pnpm (also mentioned Yarn has same issue) - PR #964: Fixed pnpm vendored deployments (this PR follows the same pattern) - Completes the vendored deployment fix for all three package managers (npm, yarn, pnpm) --- src/nodejs/supply/mocks_test.go | 14 ++++++++++++++ src/nodejs/supply/supply.go | 7 +++++++ src/nodejs/supply/supply_test.go | 29 +++++++++++++++++++++++++++++ src/nodejs/yarn/yarn.go | 10 ++++++++++ src/nodejs/yarn/yarn_test.go | 30 ++++++++++++++++++++++++++++++ 5 files changed, 90 insertions(+) diff --git a/src/nodejs/supply/mocks_test.go b/src/nodejs/supply/mocks_test.go index 3aa86e885..27d5509d7 100644 --- a/src/nodejs/supply/mocks_test.go +++ b/src/nodejs/supply/mocks_test.go @@ -245,6 +245,20 @@ func (mr *MockYarnMockRecorder) Build(buildDir, cacheDir, isBerry interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Build", reflect.TypeOf((*MockYarn)(nil).Build), buildDir, cacheDir, isBerry) } +// Rebuild mocks base method. +func (m *MockYarn) Rebuild(buildDir string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Rebuild", buildDir) + ret0, _ := ret[0].(error) + return ret0 +} + +// Rebuild indicates an expected call of Rebuild. +func (mr *MockYarnMockRecorder) Rebuild(buildDir interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Rebuild", reflect.TypeOf((*MockYarn)(nil).Rebuild), buildDir) +} + // MockStager is a mock of Stager interface. type MockStager struct { ctrl *gomock.Controller diff --git a/src/nodejs/supply/supply.go b/src/nodejs/supply/supply.go index d908a0e4b..d29e03312 100644 --- a/src/nodejs/supply/supply.go +++ b/src/nodejs/supply/supply.go @@ -46,6 +46,7 @@ type NPM interface { type Yarn interface { Build(buildDir, cacheDir string, isBerry bool) error + Rebuild(buildDir string) error } type PNPM interface { @@ -340,6 +341,12 @@ func (s *Supplier) BuildDependencies() error { } switch { + case s.UseYarn && s.IsVendored: + s.Log.Info("Prebuild detected (node_modules already exists)") + if err := s.Yarn.Rebuild(s.Stager.BuildDir()); err != nil { + return err + } + case s.UseYarn: if err := s.Yarn.Build(s.Stager.BuildDir(), s.Stager.CacheDir(), s.UsesYarnBerry); err != nil { return err diff --git a/src/nodejs/supply/supply_test.go b/src/nodejs/supply/supply_test.go index 02b59aa81..b19688814 100644 --- a/src/nodejs/supply/supply_test.go +++ b/src/nodejs/supply/supply_test.go @@ -1201,6 +1201,35 @@ var _ = Describe("Supply", func() { }) }) + Context("using yarn with vendored node_modules", func() { + BeforeEach(func() { + supplier.UseYarn = true + supplier.IsVendored = true + }) + + It("runs yarn rebuild instead of yarn build", func() { + mockYarn.EXPECT().Rebuild(buildDir).Return(nil) + Expect(supplier.BuildDependencies()).To(Succeed()) + Expect(buffer.String()).To(ContainSubstring("Prebuild detected (node_modules already exists)")) + }) + + It("runs prebuild script when specified", func() { + supplier.PreBuild = "prescriptive" + mockYarn.EXPECT().Rebuild(buildDir).Return(nil) + mockCommand.EXPECT().Execute(buildDir, gomock.Any(), gomock.Any(), "yarn", "run", "heroku-prebuild") + Expect(supplier.BuildDependencies()).To(Succeed()) + Expect(buffer.String()).To(ContainSubstring("Running heroku-prebuild (yarn)")) + }) + + It("runs postbuild script when specified", func() { + supplier.PostBuild = "descriptive" + mockYarn.EXPECT().Rebuild(buildDir).Return(nil) + mockCommand.EXPECT().Execute(buildDir, gomock.Any(), gomock.Any(), "yarn", "run", "heroku-postbuild") + Expect(supplier.BuildDependencies()).To(Succeed()) + Expect(buffer.String()).To(ContainSubstring("Running heroku-postbuild (yarn)")) + }) + }) + Describe("using npm", func() { BeforeEach(func() { supplier.UseYarn = false diff --git a/src/nodejs/yarn/yarn.go b/src/nodejs/yarn/yarn.go index 713b82a6a..83e4b2d5f 100644 --- a/src/nodejs/yarn/yarn.go +++ b/src/nodejs/yarn/yarn.go @@ -99,3 +99,13 @@ func (y *Yarn) buildClassic(buildDir, cacheDir string) error { return nil } + +func (y *Yarn) Rebuild(buildDir string) error { + y.Log.Info("Rebuilding any native modules (yarn)") + cmd := exec.Command("yarn", "rebuild") + cmd.Dir = buildDir + cmd.Stdout = y.Log.Output() + cmd.Stderr = y.Log.Output() + cmd.Env = append(os.Environ(), "npm_config_nodedir="+os.Getenv("NODE_HOME")) + return y.Command.Run(cmd) +} diff --git a/src/nodejs/yarn/yarn_test.go b/src/nodejs/yarn/yarn_test.go index d98b78beb..107d3a05f 100644 --- a/src/nodejs/yarn/yarn_test.go +++ b/src/nodejs/yarn/yarn_test.go @@ -144,4 +144,34 @@ var _ = Describe("Yarn", func() { }) }) }) + + Describe("Rebuild", func() { + var oldNodeHome string + + BeforeEach(func() { + oldNodeHome = os.Getenv("NODE_HOME") + Expect(os.Setenv("NODE_HOME", "test_node_home")).To(Succeed()) + }) + + AfterEach(func() { + Expect(os.Setenv("NODE_HOME", oldNodeHome)).To(Succeed()) + }) + + It("runs yarn rebuild with NODE_HOME set", func() { + mockCommand.EXPECT().Run(gomock.Any()).Do(func(cmd *exec.Cmd) { + Expect(cmd.Args).To(Equal([]string{"yarn", "rebuild"})) + Expect(cmd.Dir).To(Equal(buildDir)) + Expect(cmd.Env).To(ContainElement("npm_config_nodedir=test_node_home")) + }) + + Expect(y.Rebuild(buildDir)).To(Succeed()) + Expect(buffer.String()).To(ContainSubstring("Rebuilding any native modules (yarn)")) + }) + + It("propagates command errors", func() { + mockCommand.EXPECT().Run(gomock.Any()).Return(os.ErrNotExist) + + Expect(y.Rebuild(buildDir)).To(Equal(os.ErrNotExist)) + }) + }) })