pnpm: enable enableGlobalVirtualStore to share deps across worktrees#50401
Draft
manzoorwanijk wants to merge 6 commits into
Draft
pnpm: enable enableGlobalVirtualStore to share deps across worktrees#50401manzoorwanijk wants to merge 6 commits into
manzoorwanijk wants to merge 6 commits into
Conversation
Symlink node_modules from the store-wide virtual store rather than a per-checkout node_modules/.pnpm, so multiple checkouts (notably git worktrees) share one copy and `pnpm install` in a fresh worktree is near-instant. See https://pnpm.io/git-worktrees. The docs' NODE_PATH/ESM caveat only applies to hoisted dependencies, and we set `hoistPattern: []`, so nothing is hoisted and `node_modules/.pnpm/node_modules` is still never created. Note the first install after this lands purges and relinks node_modules.
With `enableGlobalVirtualStore`, node_modules is a tree of relative symlinks into the pnpm store's `links/` dir. Those links encode the checkout's depth on the host, so bind-mounting the repo at /workspace left every one of them dangling inside the container and nothing resolved. Mount the repo, and the pnpm store, at the same absolute paths the host uses, so a single set of symlinks is valid on both sides. The store no longer needs to share a mount with node_modules to preserve hardlinks, since under the global virtual store node_modules contains no hardlinked files at all -- the hardlinks live entirely inside the store. Knock-on changes: - The main .git/ is mounted at its host path too, so a worktree's `.git` file resolves natively. - Mark the repo as a git safe.directory at runtime, replacing the image's baked `/workspace` entry without needing a rebuild. - `jetpack rsync` derived the rsh-proxy script's path from the /workspace mount point; resolve it relative to the CLI instead.
The container no longer mounts the repo at /workspace, so `WORKDIR /workspace`
just left you in an empty directory and the baked `safe.directory /workspace`
covered a path that no longer exists.
`bin/monorepo` always passes `-w`, and it now marks the repo as a safe.directory
at runtime via GIT_CONFIG_*, which git honours as protected ("command" scope)
configuration.
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Tools] Development CLI
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
`enableGlobalVirtualStore` resolves packages into the pnpm store, outside the monorepo. Two bits of test tooling assumed otherwise. jest's `transformIgnorePatterns` matched the *first* `/node_modules/` in a path. That used to be the one preceding the package name, but a store path can contain its own -- CI installs pnpm under `~/setup-pnpm/node_modules/.bin/store/` -- so the next segment was `.bin/`, which is in no unignore list, and every package in the store went untransformed. ESM-only deps then blew up with `SyntaxError: Unexpected token 'export'`. Anchor to the last `/node_modules/` instead, which is the one before the package name in either layout. Vite refuses to serve files outside its allow list, which defaults to the workspace root. The Storybook vitest project now also allows the pnpm store, so `@storybook/addon-vitest`'s setup file can be imported.
github-actions
Bot
added
[JS Package] Charts
[JS Package] Storybook
[Plugin] Jetpack
`PnpmDeterministicModuleIdsPlugin` strips pnpm's paths out of module identifiers so that module IDs don't change when a package is updated. It only recognised the `node_modules/.pnpm/<pkg>@<ver>/node_modules/` layout. Under `enableGlobalVirtualStore` packages live in the store instead, as `<store>/links/<@scope|@>/<name>/<ver>/<hash>/node_modules/<path>`. That went unrecognised, so the store's absolute path -- which includes `$HOME`, and differs between a laptop and CI -- ended up hashed into every module ID. Loaders are the reason this bites even a bundle of purely local sources: `require.resolve( 'thread-loader' )` bakes an absolute realpath into each module's identifier. With this, module IDs are identical under both layouts, so the i18n-check-webpack-plugin snapshots pass unchanged.
- The monorepo container no longer hard-requires pnpm on the host. Without it there is no host store to share, and no host node_modules pointing into one, so fall back to the container's own store as before. - Mark the repo as a git safe.directory via the container's global config rather than `GIT_CONFIG_*`. The env vars are a single numbered list: ours discarded a caller's entries, and a caller's would have discarded ours. - Anchor the global-virtual-store path pattern to the store's `v<N>` version directory, so a project path that merely contains `links/<a>/<b>/<c>/<hex>/ node_modules/` is not rewritten.
anomiex
reviewed
Jul 10, 2026
anomiex
left a comment
anomiex
left a comment
Contributor
There was a problem hiding this comment.
Most of the code changes here seem fine, except for a lot of AI over-verbose comments. I still oppose the config change.
| * store-version directory keeps this from matching a project path that merely contains `links/`. | ||
| */ | ||
| const PNPM_GLOBAL_STORE_PATH_REGEXP = | ||
| /(?<=^|[|!])[^|!]*?\/v[0-9]+\/links\/[^/]+\/[^/]+\/[^/]+\/[0-9a-f]{32,}\/node_modules\/([^|!]+)/g; |
Contributor
There was a problem hiding this comment.
Should the [^/] in here be [^/!|]?
I suppose the same goes for the one above too.
You might also do like \/links\/@[^/]*\/ since, according to the comment above, that component always begins with an @.
Comment on lines
+19
to
+22
| * With `enableGlobalVirtualStore`, packages live in the pnpm store instead of `node_modules/.pnpm`, | ||
| * as `<store>/v<N>/links/<@scope|@>/<name>/<version>/<hash>/node_modules/<path>`. The store is | ||
| * outside the monorepo, so its absolute path would otherwise leak into the identifier. The `v<N>` | ||
| * store-version directory keeps this from matching a project path that merely contains `links/`. |
Contributor
There was a problem hiding this comment.
AI over-verbosity.
Suggested change
| * With `enableGlobalVirtualStore`, packages live in the pnpm store instead of `node_modules/.pnpm`, | |
| * as `<store>/v<N>/links/<@scope|@>/<name>/<version>/<hash>/node_modules/<path>`. The store is | |
| * outside the monorepo, so its absolute path would otherwise leak into the identifier. The `v<N>` | |
| * store-version directory keeps this from matching a project path that merely contains `links/`. | |
| * With `enableGlobalVirtualStore`, packages live at | |
| * `<store>/v<N>/links/<@scope|@>/<name>/<version>/<hash>/node_modules/<path>`. |
Comment on lines
+140
to
+145
| # `enableGlobalVirtualStore` makes node_modules a tree of relative symlinks into the store's | ||
| # `links/` dir, so the store has to sit at the same absolute path inside the container as it does | ||
| # on the host. Together with mounting the repo at its host path (below), that keeps one set of | ||
| # symlinks valid on both sides. | ||
| # Without pnpm on the host there's no host store to share, and no host node_modules pointing into | ||
| # one. Let the container fall back to its own store, as it did before. |
Contributor
There was a problem hiding this comment.
This one seems overly verbose too.
| # symlinks valid on both sides. | ||
| # Without pnpm on the host there's no host store to share, and no host node_modules pointing into | ||
| # one. Let the container fall back to its own store, as it did before. | ||
| PNPM_STORE_PATH="$( cd "$MONOREPO_ROOT" && command -v pnpm >/dev/null 2>&1 && pnpm store path 2>/dev/null )" |
Contributor
There was a problem hiding this comment.
Since this script is using bash, we can simplify this.
Suggested change
| PNPM_STORE_PATH="$( cd "$MONOREPO_ROOT" && command -v pnpm >/dev/null 2>&1 && pnpm store path 2>/dev/null )" | |
| PNPM_STORE_PATH="$( cd "$MONOREPO_ROOT" && command -v pnpm &>/dev/null && pnpm store path 2>/dev/null )" |
| if [[ "$IS_WORKTREE" = "1" ]]; then | ||
| echo "[pnpm] store dir (host): $DOCKER_DATA_DIR/.pnpm-store" | ||
| fi | ||
| echo "[pnpm] store path (host and container): ${PNPM_STORE_PATH:-<container default>}" |
Contributor
There was a problem hiding this comment.
Remember the variable may be unset. This might print like
[pnpm] store path (host and container): :-<container default>
That may be confusing.
Comment on lines
+168
to
+170
| # Mark the repo as a safe directory for git, which otherwise refuses to operate on a checkout owned | ||
| # by another user. `$DOCKER_DATA_DIR` is the container's HOME, so this lands in git's global config; | ||
| # passing it via `GIT_CONFIG_*` instead would clobber, and be clobbered by, a caller's own entries. |
Contributor
There was a problem hiding this comment.
Suggested change
| # Mark the repo as a safe directory for git, which otherwise refuses to operate on a checkout owned | |
| # by another user. `$DOCKER_DATA_DIR` is the container's HOME, so this lands in git's global config; | |
| # passing it via `GIT_CONFIG_*` instead would clobber, and be clobbered by, a caller's own entries. | |
| # Mark the repo as a safe directory for git, which otherwise refuses to operate on a checkout owned | |
| # by another user. `$DOCKER_DATA_DIR` is the container's HOME, so this lands in git's global config. |
| // - @gravatar-com: for the lifted Gravatar component's hovercard styles | ||
| // - marked: esm-only | ||
| // - uuid: v14 went esm-only, so it needs transforming | ||
| // `(?!.*/node_modules/)` anchors to the last `node_modules`, the one before the package name. The pnpm store's own path may contain one too. |
Contributor
There was a problem hiding this comment.
Suggested change
| // `(?!.*/node_modules/)` anchors to the last `node_modules`, the one before the package name. The pnpm store's own path may contain one too. | |
| // `(?!.*/node_modules/)` handles pnpm's store with nested `node_modules` dirs. |
Comment on lines
+27
to
+31
| # Symlink node_modules from a store-wide virtual store instead of node_modules/.pnpm, so | ||
| # checkouts (notably git worktrees) share one copy. https://pnpm.io/git-worktrees | ||
| # The docs' NODE_PATH/ESM caveat only affects hoisted deps, and we hoist nothing. | ||
| enableGlobalVirtualStore: true | ||
|
|
Contributor
There was a problem hiding this comment.
I still think that if people want this, they probably want it for everything and should set it in their global config. Meanwhile, not setting it here will avoid breaking people's workflows that make use of node_modules/.pnpm/ having everything, as discussed in #48093.
Suggested change
| # Symlink node_modules from a store-wide virtual store instead of node_modules/.pnpm, so | |
| # checkouts (notably git worktrees) share one copy. https://pnpm.io/git-worktrees | |
| # The docs' NODE_PATH/ESM caveat only affects hoisted deps, and we hoist nothing. | |
| enableGlobalVirtualStore: true |
Contributor
|
Other references to
|
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
2 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.
Another attempt for #48093
Proposed changes
Enables pnpm's global virtual store so separate checkouts — git worktrees in particular — share one set of installed packages instead of each building their own
node_modules/.pnpm.node_modulesbecomes symlinks into the pnpm store:du -sh node_modulespnpm installin a fresh worktree(Install times measured in a fresh worktree of this repo against a warm store.)
Because packages now resolve to the store, outside the repo, three things that assumed otherwise needed fixing:
/workspace. The store symlinks are relative and encode the checkout's depth on the host, so at/workspacethey all dangled. It now mounts the repo, the store, and the main.git/at their host paths. The old/workspace/.pnpm-storevs/root/.pnpm-storesplit is gone:node_modulesno longer contains hardlinked files, so the store doesn't need to share its mount.transformIgnorePatternsmatched the first/node_modules/in a path. CI installs pnpm under~/setup-pnpm/node_modules/.bin/store/, so that matched the store's own path, and every package in it went untransformed — ESM-only deps failed withSyntaxError: Unexpected token 'export'. Now anchored to the last/node_modules/, the one before the package name, which is correct in either layout.Two smaller knock-ons:
jetpack rsynchad the rsh-proxy path hardcoded to/workspace, and the image'sWORKDIR /workspace/ bakedsafe.directory /workspaceare now dead (the container setssafe.directoryat runtime instead).Notes on the docs' caveats:
hoistPattern: [], so nothing is hoisted.pnpm-workspace.yaml. CI is fine anyway —actions/setup-nodecaches the store, notnode_modules.pnpm installafter this merges purges and relinksnode_modules. Interactively pnpm prompts; in a non-TTY it aborts withERR_PNPM_ABORTED_REMOVE_MODULES_DIR_NO_TTY, so scripted installs need--config.confirmModulesPurge=false. One-time, but worth flagging so it doesn't read as a broken install.Related product discussion/links
enableGlobalVirtualStoreDoes this pull request change what data or activity we track or use?
No.
Testing instructions
Install and worktrees
pnpm install, confirming thenode_modulesremoval prompt.readlink node_modules/huskyshould point into your pnpm store, andnode_modules/.pnpm/node_modulesshould not exist.git worktree add --detach /tmp/wt-test HEAD && cd /tmp/wt-test && time pnpm install— should finish in seconds.node -e "require.resolve('husky')"in both. Clean up withgit worktree remove --force /tmp/wt-test.Monorepo container
Most likely thing to regress, since
node_modulesis bind-mounted in. Needs an image with the pinned pnpm 11.5.2 — older ones ship pnpm 10, which ignorespnpm_config_store_dir(already a no-op before this PR).docker pull automattic/jetpack-monorepo:latest, or let the 24-hour image check refresh it.tools/docker/bin/monorepo bash -c 'pwd; node -e "require.resolve(\"husky\")" && echo OK'—pwdprints the repo's host path, resolution succeeds.tools/docker/bin/monorepo git status— no "dubious ownership" error.pnpm store pathon the host andtools/docker/bin/monorepo pnpm store pathshould match.tools/docker/bin/monorepo pnpm install, thenpnpm installon the host: the second should be a fast no-op, no purge prompt, no lockfile churn..gitmounts.Tests
pnpm exec jestinprojects/js-packages/componentsandprojects/js-packages/charts.pnpm run storybook:testinprojects/js-packages/storybook(build first — stalebuild-moduleartifacts cause unrelated failures).Other
jetpack rsyncwithRSYNC_PROXY_SOCKET, confirm a sync still works.tools/docker/bin/monorepo git statussucceeds. Docker Desktop on macOS maps bind-mount ownership to the container user, so git's "dubious ownership" check never fires there and the runtimesafe.directoryentry can't be exercised locally.