Conversation
`pnpm fetch` writes empty `hoistPattern`/`publicHoistPattern` into .modules.yaml because the virtualStoreOnly install path skips hoisting. v11 removed the explicit-config gate in `validateModules`, so the follow-up `pnpm install` saw those forced-empty values as a hoist-pattern change and purged node_modules. Mark .modules.yaml with `virtualStoreOnly: true` after a fetch so the next install can recognize the incomplete state, skip the hoist-pattern comparison, and complete the missing post-import linking in place. A real hoist-pattern change still triggers the purge as before. Closes #11488
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFetch now records a virtualStoreOnly flag in the modules manifest; manifest writing and validation logic use this flag so a subsequent install skips hoist-pattern comparisons and does not recreate node_modules. Adds a regression test and changeset referencing issue ChangesVirtual Store-Only Flag & Fetch→Install Flow
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Fetch as pnpm fetch
participant Manifest as modules.yaml
participant Install as pnpm install
participant Validator as validateModules
Dev->>Fetch: run `pnpm fetch`
Fetch->>Manifest: write modules manifest with `virtualStoreOnly: true` (rgba(30,144,255,0.5))
Dev->>Install: run `pnpm install --frozen-lockfile`
Install->>Manifest: read modules manifest
Install->>Validator: pass modules.virtualStoreOnly flag
Validator-->>Install: skip hoist-pattern diff checks when virtualStoreOnly is true (rgba(50,205,50,0.5))
Install->>Dev: complete install without recreating node_modules
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
installing/commands/test/fetch.ts (1)
233-277: ⚡ Quick winAdd an assertion that
virtualStoreOnlyis cleared after install.This regression test is strong; asserting manifest cleanup would fully cover the intended state transition (fetch marks it, install removes it).
Proposed test extension
expect(fs.statSync(virtualStoreDir).ino).toBe(virtualStoreInodeBefore) // The package symlink must be present after install completes the linking. expect(fs.existsSync(path.resolve(project.dir(), 'node_modules/is-positive'))).toBeTruthy() + const modulesManifest = JSON.parse( + fs.readFileSync(path.resolve(project.dir(), 'node_modules/.modules.yaml'), 'utf8') + ) + expect(modulesManifest.virtualStoreOnly).toBeUndefined() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@installing/commands/test/fetch.ts` around lines 233 - 277, Extend the test 'install after fetch does not recreate node_modules' to assert that the transient fetch marker is removed: after the second install.handler call, read the modules metadata that fetch sets (the .modules.yaml / virtual store manifest associated with virtualStoreDir) and assert that the virtualStoreOnly flag is no longer present or is false; use the existing virtualStoreDir and project variables to locate the manifest and add an expect(...) assertion verifying virtualStoreOnly is cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@installing/commands/test/fetch.ts`:
- Around line 233-277: Extend the test 'install after fetch does not recreate
node_modules' to assert that the transient fetch marker is removed: after the
second install.handler call, read the modules metadata that fetch sets (the
.modules.yaml / virtual store manifest associated with virtualStoreDir) and
assert that the virtualStoreOnly flag is no longer present or is false; use the
existing virtualStoreDir and project variables to locate the manifest and add an
expect(...) assertion verifying virtualStoreOnly is cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1856d89-9fe8-4069-8f1f-1af97edcc2ac
📒 Files selected for processing (5)
.changeset/fetch-install-no-recreate.mdinstalling/commands/test/fetch.tsinstalling/deps-installer/src/install/validateModules.tsinstalling/deps-restorer/src/index.tsinstalling/modules-yaml/src/index.ts
The previous test only verified that node_modules wasn't purged and the direct dep was symlinked. Strengthen it with @pnpm.e2e/pkg-with-1-dep so we also assert the transitive dep ends up under .pnpm/node_modules — i.e. the install actually completes the post-import linking that fetch skips, not just leaves the modules dir in place.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a pnpm v11 regression where running pnpm install after pnpm fetch unnecessarily purges/recreates node_modules due to .modules.yaml containing forced-empty hoistPattern / publicHoistPattern values from the fetch’s virtualStoreOnly install path.
Changes:
- Persist a new
.modules.yamlflag (virtualStoreOnly: true) for fetch/virtual-store-only installs and avoid comparing hoist patterns during the next install’svalidateModulesstep. - Propagate
virtualStoreOnlyinto the modules manifest writer in the headless install path and ensure the field is omitted when not set. - Add a regression test to assert
node_modules/.pnpmis not recreated across a fetch → install sequence.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| installing/modules-yaml/src/index.ts | Adds the virtualStoreOnly field to the modules manifest type and strips it on write when not enabled. |
| installing/deps-restorer/src/index.ts | Passes virtualStoreOnly through to writeModulesManifest() in the headless install writer. |
| installing/deps-installer/src/install/validateModules.ts | Skips hoist/public-hoist pattern mismatch checks when .modules.yaml is marked virtualStoreOnly. |
| installing/commands/test/fetch.ts | Adds regression test ensuring install after fetch preserves the virtual store directory and links dependencies. |
| .changeset/fetch-install-no-recreate.md | Documents the fix and bumps affected packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
676
to
680
| virtualStoreDir, | ||
| virtualStoreDirMaxLength: opts.virtualStoreDirMaxLength, | ||
| allowBuilds: opts.allowBuilds, | ||
| virtualStoreOnly: opts.virtualStoreOnly, | ||
| }) |
Member
Author
There was a problem hiding this comment.
Good catch — confirmed and fixed in 54ee7bc.
installing/deps-installer/src/install/index.ts:1593 (the non-headless write path) was indeed propagating virtualStoreOnly: true from ctx.modulesFile. Reproduced manually: after pnpm fetch followed by pnpm install --no-frozen-lockfile (which forces resolution and exits the headless path), .modules.yaml still carried the marker. Now overridden explicitly with opts.virtualStoreOnly, which is undefined for normal installs — so writeModulesManifest's falsy-strip drops it from the file.
building/after-install/src/index.ts doesn't need the same change: it re-reads .modules.yaml via getContext after deps-installer has already cleared the flag, so the spread sees the cleared value. Verified by running the headless and non-headless install paths after fetch — both end with no virtualStoreOnly in .modules.yaml. Test extended to assert the clear-on-install behavior.
Written by an agent (Claude Code, claude-opus-4-7).
Both keys are already declared in install's rcOptionsTypes but were missing from the InstallCommandOptions Pick, so callers that build the option object programmatically (rather than via the Config loader) had no typed way to set them.
The non-headless install path in deps-installer writes .modules.yaml by spreading ctx.modulesFile, which carried the virtualStoreOnly: true flag forward when the previous step was a fetch. Future installs would then keep skipping hoist-pattern checks even though the install was complete. Override the field explicitly with opts.virtualStoreOnly so the marker is cleared whenever a regular install runs after fetch. Also assert the clear-on-install behavior in the regression test.
zkochan
added a commit
that referenced
this pull request
May 6, 2026
Closes #11488. `pnpm fetch` writes forced-empty `hoistPattern: []` and `publicHoistPattern: []` into `.modules.yaml` (because its `virtualStoreOnly` install path skips hoisting). In v10 the follow-up `pnpm install` ignored these unless the user had explicitly set a hoist-pattern in their config. v11's [#11199](#11199) removed that explicit-config gate, so `validateModules` now always sees the empty patterns as a hoist-pattern change and purges `node_modules` — slow on every CI run, and per the bug report sometimes leaves the modules dir in an `ERR_MODULE_NOT_FOUND` state on subsequent runs. The fix marks `.modules.yaml` with a new `virtualStoreOnly: true` field after a fetch. `validateModules` recognizes this flag as "incomplete install state" and skips the `PUBLIC_HOIST_PATTERN_DIFF` / `HOIST_PATTERN_DIFF` comparisons. The next install then completes the missing post-import linking in place rather than purging. The flag is dropped from `.modules.yaml` once a normal install runs. A genuine hoist-pattern change (without a fetch in between) still triggers the purge as before — verified manually with `publicHoistPattern` in `pnpm-workspace.yaml`.
7 tasks
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
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
Closes #11488.
pnpm fetchwrites forced-emptyhoistPattern: []andpublicHoistPattern: []into.modules.yaml(because itsvirtualStoreOnlyinstall path skips hoisting). In v10 the follow-uppnpm installignored these unless the user had explicitly set a hoist-pattern in their config. v11's #11199 removed that explicit-config gate, sovalidateModulesnow always sees the empty patterns as a hoist-pattern change and purgesnode_modules— slow on every CI run, and per the bug report sometimes leaves the modules dir in anERR_MODULE_NOT_FOUNDstate on subsequent runs.The fix marks
.modules.yamlwith a newvirtualStoreOnly: truefield after a fetch.validateModulesrecognizes this flag as "incomplete install state" and skips thePUBLIC_HOIST_PATTERN_DIFF/HOIST_PATTERN_DIFFcomparisons. The next install then completes the missing post-import linking in place rather than purging. The flag is dropped from.modules.yamlonce a normal install runs.A genuine hoist-pattern change (without a fetch in between) still triggers the purge as before — verified manually with
publicHoistPatterninpnpm-workspace.yaml.Test plan
install after fetch does not recreate node_modulesininstalling/commands/test/fetch.ts— assertsnode_modules/.pnpm's inode is preserved across fetch→install and the package symlink exists afterwards.fetch --prod+install --offline --prod --frozen-lockfile) against the rebuilt bundle: no moreRecreating /private/tmp/.../node_modules,pnpm startworks, modules dir is preserved.publicHoistPatterninpnpm-workspace.yamlbetween two installs (no fetch involved) still triggers the purge.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
New Features
Tests