Repository navigation
Framework: Switch from hoisted to isolated dependencies - #83847
Conversation
🤖 PR meta 🤖🎉 PropsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Updated as activity occurs, without notifying anyone named here. Add the 📦 Bundle sizeSize Change: +586 B (+0.01%) Total Size: 8.25 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsSome tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information. three users concurrently edit a large post with diverse blocks in
|
e1e508d to
3de7c68
Compare
| @@ -1,3 +1,4 @@ | |||
| install-strategy = "linked" | |||
There was a problem hiding this comment.
npm 11.16 is still allowed, but its linked installation leaves webpack unable to find the installed webpack-cli and breaks the documented webpack command. Could we raise both npm minimum entries to >=11.18.0 and verify that minimum in CI?
There was a problem hiding this comment.
Yeah, that makes sense. The expectation, though, is that everyone uses the latest npm version, which the CI already does.
There was a problem hiding this comment.
We're technically not checking on CI that the min required version works well (since it installs latest version), but I think that's ok and not a blocker
87702b4 to
ead4f75
Compare
Dependencies are installed into node_modules/.store/ and symlinked to the packages that declare them, so a package can no longer resolve a dependency it never declared.
typeRoots drops its repo-root @types fallback, so a package using types it never declared fails instead of borrowing them. Jest transforms through .store/, and test-create-block gives wp-scripts a NODE_PATH fallback for the bins it shells out to. Storybook roots Vite at the repository root by default, which under the isolated layout owns none of the dependencies its smoke tests resolve. CI skips the node_modules cache on Windows, whose tar cannot restore the isolated layout's links.
The linked install strategy is stable from v11.18, so the engine range no longer accepts the versions that shipped it as experimental.
The lockfile is identical under either layout and the installed-deps check reduces paths to name@version, so a tree installed with the other strategy satisfied every comparison and the mtime fast path skipped the check entirely. Compare npm's configured install-strategy against the tree on disk first, so switching strategies without a lockfile change still asks for a reinstall.
Rooting Vite at the Storybook workspace left the docgen plugin's default glob stopping at that root, so prop tables covered 298 components instead of 1079. Naming the documented sources restores them; excluding stories would drop the components they define. The root now derives from each config's own directory, so the Playwright config no longer inherits this workspace as its root. Its docgen globs move with it, matching the story globs beside them.
The fallback worked around `@wordpress/scripts` resolving its default browserslist config from the consumer's cwd, which #83902 fixes in the package itself by passing the preset's queries to webpack.
ccdc909 to
4564123
Compare
ciampo
left a comment
There was a problem hiding this comment.
LGTM 🚀
Congratulations for working all the way to this moment 🎉
|
I'm sure @manzoorwanijk is also relieved to no longer have to track down conflicts that folks have added over the past several months 😂 Great work here! 🎉 |
Definitely 😁
Thanks |
…ode (#588) ## Why Installing a new Gutenberg site fails at the npm install step with `Error: Digest method not supported`. Gutenberg set `install-strategy = "linked"` in its `.npmrc` (WordPress/gutenberg#83847), so npm now uses its isolated installer, which names every `node_modules/.store` directory with `crypto.createHash('shake256')`. The app runs npm on Electron's Node, whose BoringSSL has no SHAKE256, so the install throws before it writes anything. ## What changes - `src/electron-node-compat.js` gets a pure-JS SHAKE256 (FIPS 202) and `provideShake256()`, which routes `createHash('shake256')` to it **only when the runtime lacks it**. Every other algorithm, and plain Node, is untouched. - The shim preload applies it, which covers any `npm`/`npx` reached through the shims, including the Windows spawn-patch route. - `src/install-runner.js` applies it before requiring npm. This is the route the bug hit: the runner loads npm in-process and never gets the preload. It is the real hash, not a stand-in, so the `.store` names match what a contributor gets on plain Node. ## How to test this Platforms: any. Build: the Buildkite artifact for the current head. **Starting state:** no Gutenberg site in the app. 1. Create a new Gutenberg site. 2. Let the npm install step run. - Expected: it finishes without `Digest method not supported`, and `node_modules/.store` exists in the site folder. 3. On an existing `wordpress-develop` site (not linked), run npm install again. - Expected: it succeeds as before. **What must not have happened:** the `wordpress-develop` install changing behaviour. It does not use the linked strategy, so it never asks for SHAKE256. Covered by `a linked install succeeds on the runtime the app runs npm on` in `tests/unit/npm-install.integration.test.cjs`. Under `npm run test:electron` it fails with the old `install-runner.js` and passes with this change (checked). Run from the repository root. ## Risks and limitations - Only `createHash('shake256')` is filled in: `update()` and one `digest()`, which is all npm uses. `copy()`, stream use and `crypto.hash()` are not, and SHA-3 is not. - Separate from this fix: npm 11.20 skipped the install scripts for `esbuild`, `fsevents`, `nx` and others on a full Gutenberg install (`npm install-scripts approve`). Not investigated here. ## Related Fixes #586 --- <details> <summary>Design decisions and alternatives considered</summary> Forcing `npm_config_install_strategy=hoisted` in the child environment was a one-line alternative. Rejected because it overrides the layout Gutenberg chose on purpose, so the app's `node_modules` would differ from Gutenberg's CI. The hash is applied only after probing the runtime, so a future Electron that gains SHAKE256 uses its own. </details> <details> <summary>Review outcome (required, see AGENTS.md)</summary> 1 [fix here] · 0 [follow-up]: fixed. - **Review:** completed. Separate-context subagent against `.github/instructions/code-review.instructions.md`; `npm run lint` and `npm test` clean, `npm run test:electron` clean. - Fixed: the install-runner route had no test that would fail without the fix. Added the `linked-install` fixture and the integration test above. - Also taken: the capability probe passes `outputLength`, so it does not lean on Node's default output length. - The review also caught that the branch had first been cut from the wrong base. It was rebuilt from `trunk` before committing; this PR contains one commit. - **Since review:** the fixes above, re-verified with lint and both test runtimes. </details> <details> <summary>Implementation notes</summary> - Checked against native `crypto.createHash('shake256')` on Node 20 for inputs and outputs on both sides of every 136-byte block boundary, with input split across `update()` calls: no mismatches. - Keccak-f[1600] works on 32-bit lane halves, because BigInt is too slow. About 40 MB/s on Electron, far beyond what npm hashes. - `no-bitwise` is disabled for the Keccak block only, with a reason. - Verified end to end: the real install runner on Electron 43.6.0 installs a current Gutenberg checkout (1886 `.store` entries). </details> Nothing on screen changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
) * Framework: Switch to npm install-strategy=linked Dependencies are installed into node_modules/.store/ and symlinked to the packages that declare them, so a package can no longer resolve a dependency it never declared. * Framework: Adapt tooling to the isolated install layout typeRoots drops its repo-root @types fallback, so a package using types it never declared fails instead of borrowing them. Jest transforms through .store/, and test-create-block gives wp-scripts a NODE_PATH fallback for the bins it shells out to. Storybook roots Vite at the repository root by default, which under the isolated layout owns none of the dependencies its smoke tests resolve. CI skips the node_modules cache on Windows, whose tar cannot restore the isolated layout's links. * Framework: Require npm v11.18 The linked install strategy is stable from v11.18, so the engine range no longer accepts the versions that shipped it as experimental. * Build: Detect a node_modules installed with the other strategy The lockfile is identical under either layout and the installed-deps check reduces paths to name@version, so a tree installed with the other strategy satisfied every comparison and the mtime fast path skipped the check entirely. Compare npm's configured install-strategy against the tree on disk first, so switching strategies without a lockfile change still asks for a reinstall. * Storybook: Restore docgen coverage under the isolated layout Rooting Vite at the Storybook workspace left the docgen plugin's default glob stopping at that root, so prop tables covered 298 components instead of 1079. Naming the documented sources restores them; excluding stories would drop the components they define. The root now derives from each config's own directory, so the Playwright config no longer inherits this workspace as its root. Its docgen globs move with it, matching the story globs beside them. * Framework: Drop the NODE_PATH fallback in create-block validation The fallback worked around `@wordpress/scripts` resolving its default browserslist config from the consumer's cwd, which WordPress#83902 fixes in the package itself by passing the preset's queries to webpack. Co-authored-by: manzoorwanijk <manzoorwanijk@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
|
I had a conversation with an agent today while working on a pull request, and it said something that stood out to me in relation to isolated dependencies:
Further prompting confirmed that it got tripped up by the isolated dependencies, compounded by my own agent guidance to manually install dependencies:
Might be worth a blurb in |
What?
Closes #76195
Alternative to #74689
Sets
install-strategy = "linked"in.npmrcto isolate dependencies so every workspace resolves only the dependencies it actually declares.Why?
We couldn't change the package manager, so I changed npm!
In January I proposed switching to pnpm (#74689). Not for novelty. Gutenberg publishes 80+ packages, and npm's hoisted layout lets any of them import a module they never declared. It works in the monorepo, CI stays green, the package ships, and then it breaks in somebody else's project. Cataloguing that failure turned up 60+ issues spanning six years, all the same shape: missing
react, missing@babel/runtime, missing peers, missing siblings.The community pushed back, and it was right to. Switching package managers needs coordination across repositories, older release branches would need updating too, and it asks every contributor to install and juggle a second package manager for two WordPress projects. Those are real costs, and a Make Core proposal was the correct bar to set. The proposal never moved beyond community review.
So instead of pushing a migration uphill, I took the pushback as the brief: get npm itself to do the thing pnpm was wanted for.
npm already had an isolated install strategy. It had simply never been run in anger against a monorepo this size, and it showed. Fixing it took 70+ PRs and 60+ bug reports in npm/cli, and 3 accepted RFCs, alongside 40+ merged PRs here untangling this repo's own root dependencies. The strategy graduated from experimental to stable in npm v11.18 (npm/cli#9674), and npm's own documentation now recommends it to package authors for precisely our problem:
Then #82370 brought Node 24 and npm 11, and the last blocker in #76195 went with it.
That is why this is a config change rather than a migration. The hard part is upstream, shipped, and stable, and every npm user benefits from it rather than just this repo.
The exploration was done in #75814.
How?
.npmrcsetsinstall-strategy = "linked". Dependencies land innode_modules/.store/and are symlinked to exactly the packages that declare them.patch-packageis left as trunk has it. The repo has no patches left, and with none to apply it runs clean under this layout. It could not work if that changed (Support npminstall-strategy=linked(.storedirectory layout) ds300/patch-package#595): it resolves packages at nestednode_modules/paths the linked strategy never creates, and refuses to diff a package directory containing symlinks into the store. A replacement that does work, withpatch applyonpostinstallandpatch create <package>to author one, plus tests, sits in Explore using npm install-strategy=linked #75814 undertools/monorepo/patchingand can be copied across if a patch is ever needed before npm v12's nativenpm patchlands (Framework: Update npm to v12 #82328).typeRootsdrops its repo-root@typesfallback, so a package using types it never declared fails instead of borrowing them. Storybook roots Vite at its own workspace rather than the repo root.node_modulescache on Windows, whosetarcannot restore the isolated layout's links.Testing Instructions
nvm use && rm -rf node_modules && npm ci. Confirmnode_modules/.store/exists and each dependency under it is symlinked from the packages that declare it.npm run build,npm run typecheck,npm run test:unit:vitest,npm run storybook:build.Use of AI Tools
Claude Code was used to validate this branch against Node 24 / npm 11 and to draft this description. All changes were reviewed by me.