Conversation
Even with shamefully-hoist=true, pnpm's symlink-based layout does not consistently hoist every transitive dependency to the project root. Flow's frontend tooling and bundled Vite plugins (e.g. the React function location plugin) import transitive deps directly — when one of those is left only inside .pnpm/..., Node ESM cannot resolve it, Vite fails or returns broken modules, and Selenium tests hang waiting for pages that never render. The previous commit pinned @babel/types explicitly to fix that one case; missing-dep failures kept appearing on different transitives (cookie, set-cookie-parser, @lit/reactive- element, @preact/signals-react) as the dep graph shifts. Switch the bundled .npmrc template to node-linker=hoisted, which makes pnpm install in flat npm-style mode: every package lives at the project root, no .pnpm/ virtual store. All transitive resolutions become deterministic. Slightly larger node_modules and slower install (seconds, not minutes), but eliminates this whole class of flake. shamefully-hoist=true is left in the template since it's now redundant in hoisted mode but harmless, and removing it would change behavior for users with custom .npmrc files that lack node-linker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Setting node-linker=hoisted in .npmrc alone did not flip pnpm's actual install layout; node_modules/.pnpm/ kept appearing in CI runs and transitive deps stayed unreachable from the project root. The --shamefully-hoist=true CLI flag added by getPnpmExecutable() likely took precedence and locked pnpm to the isolated layout (where shamefully-hoist makes sense as a partial-hoist heuristic). Replace --shamefully-hoist=true with --config.node-linker=hoisted on the CLI. CLI > .npmrc > defaults in pnpm's config precedence, so this forces flat npm-style install regardless of project-level config. The .npmrc still keeps shamefully-hoist=true and node-linker=hoisted for manual pnpm invocations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Artur-
reviewed
May 8, 2026
Member
Artur-
left a comment
There was a problem hiding this comment.
Looks like a good workaround but why do we need to add the same parameter to both npmrc and the command?
Artur-
approved these changes
May 8, 2026
Member
|
Let's simplify in another PR - this unblocks everything else |
Artur-
pushed a commit
that referenced
this pull request
May 8, 2026
## Summary
The Vite dev server intermittently fails (or hangs) on `vite-basics` and
other Vite-using IT modules because transitive npm dependencies are not
consistently reachable from the project's root `node_modules`. Selenium
tests then wait for pages that never render, and the matrix shard times
out at the 30-minute job cap.
Two distinct symptoms observed:
1. **`Cannot find package '@babel/types'`** — the bundled Vite plugin
`react-function-location-plugin.js` does
`import * as t from '@babel/types';`, but `@babel/types` was only
present transitively via `@babel/core` and pnpm did not always hoist
it to the root.
2. **`Failed to resolve import "@lit/reactive-element/..."`,
`"cookie"`, `"set-cookie-parser"`, `"@preact/signals-react/runtime"`** —
all transitives of declared deps (`lit`, `react-router`,
`@preact/signals-react-transform`) — also not consistently hoisted.
Both are the same underlying problem: pnpm with `shamefully-hoist=true`
does not deterministically expose every transitive at the project root.
## Fix
Three commits, layered:
### 1. Declare `@babel/types` directly
Adds `"@babel/types": "7.29.0"` (matching `@babel/core`) to the bundled
`devDependencies` in
`flow-server/.../dependencies/vite/package.json`. Correct on its own
merits — the plugin imports the package, so the package should be a
real dependency. Updates `NodeUpdaterTest`'s expected-set accordingly.
### 2. Switch the bundled `.npmrc` template to `node-linker=hoisted`
Adds `node-linker=hoisted` to `flow-server/src/main/resources/npmrc`,
the template `TaskRunNpmInstall` writes to the project. This tells
pnpm to install in **flat npm-style mode** — every package (declared
or transitive) at the project root, no `.pnpm/` virtual store.
Resolutions become deterministic.
`shamefully-hoist=true` is left in the template alongside, so manual
`pnpm install` invocations from a developer shell still pick up at
least the legacy partial-hoist behavior if some user-level config
overrides our `node-linker`.
### 3. Pass `--config.node-linker=hoisted` on the pnpm CLI
The `.npmrc` change alone did not actually flip pnpm's install layout
in CI: `node_modules/.pnpm/...` paths kept appearing in error
messages, indicating pnpm was still using its default `isolated`
mode. The pre-existing `--shamefully-hoist=true` CLI flag added by
`FrontendTools.getPnpmExecutable()` likely took precedence and locked
pnpm to the isolated layout (where `shamefully-hoist` makes sense as
a partial-hoist heuristic).
Replace `--shamefully-hoist=true` with `--config.node-linker=hoisted`
on the CLI. CLI > `.npmrc` > defaults in pnpm's config precedence, so
the install is now unambiguously hoisted regardless of any project-
or user-level `.npmrc` content. `FrontendToolsTest` updated to assert
the new flag.
## Trade-offs
- **Disk/install time**: hoisted mode uses slightly more disk and is
a few seconds slower than the symlink store for fresh installs.
Acceptable for the determinism gain.
- **Loose isolation**: hoisted mode allows packages to import any
other package, declared or not. Flow's frontend code already relies
on this (e.g. plugins importing transitives), so this is the
current de-facto behavior — just made deterministic.
- **Behavioral change for downstream users**: anyone with a custom
`.npmrc` that lacks the auto-gen marker keeps their config (per
existing `TaskRunNpmInstall.createNpmRcFile()` logic), but the new
CLI flag still applies — they'll silently get hoisted mode for
Flow-driven installs. Worth flagging in release notes.
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 8, 2026
ZheSun88
added a commit
that referenced
this pull request
May 8, 2026
… (CP: 25.1) Cherry-pick of d9290f8 from main. Adjusted for 25.1's dep-graph: @babel/types is pinned to 7.28.5 to match @babel/preset-react's version on this branch (main pins to 7.29.0 to match @babel/core which is not declared on 25.1). @babel/core and @babel/plugin-transform-react-jsx-development from the main commit are not added — they are not declared as direct deps on 25.1 and the React function location plugin only imports @babel/types. The .npmrc and FrontendTools changes that switch pnpm to hoisted mode apply unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ZheSun88
added a commit
that referenced
this pull request
May 8, 2026
… (CP: 25.0) (#24296) Cherry-pick of #24288 (commit d9290f8) from main to 25.0. ## Summary Same fix as #24288: switches pnpm install to hoisted (flat npm-style) layout so transitive npm deps (`@babel/types`, `@lit/reactive-element`, `cookie`, `set-cookie-parser`, `@preact/signals-react/runtime`) are always reachable from project root, eliminating the `vite-basics` IT hang and similar symptoms. ## Adjustments for 25.0 This branch is pre-`flow-build-tools` refactor (#23161), so all the Java files the original commit touches (`FrontendTools.java`, `FrontendToolsTest.java`, `NodeUpdaterTest.java`, `TaskRunPnpmInstallTest.java`) live under `flow-server/` here. The edits are otherwise identical. Two version adjustments to match this branch's dep graph: 1. **`@babel/types` pinned to `7.28.5`** (matching `@babel/preset-react` already in `vite/package.json`) instead of main's `7.29.0`. 25.0 does not declare `@babel/core`, so version coherence is anchored to `preset-react`. 2. **Skipped `@babel/core` and `@babel/plugin-transform-react-jsx-development`** declarations from the main commit — not declared as direct deps on 25.0, and the React function location plugin only imports `@babel/types`. Everything else is identical: - `flow-server/.../npmrc`: adds `node-linker=hoisted`. - `FrontendTools.java`: replaces `--shamefully-hoist=true` CLI flag with `--config.node-linker=hoisted`. - All three test files updated to match the new assertions. ## Test plan - [ ] CI on this PR passes (specifically `it-tests (1)` and `it-tests (2)`). - [ ] `mvn -pl flow-server test -Dtest=NodeUpdaterTest,TaskRunPnpmInstallTest,FrontendToolsTest` passes. - [ ] After install in any vite-using IT module, `node_modules/.pnpm/` does not exist (or is empty); `node_modules/@babel/types/` and `node_modules/@lit/reactive-element/` exist as real directories at the project root. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Zhe Sun <zhe@vaadin.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ZheSun88
added a commit
that referenced
this pull request
May 8, 2026
… (CP: 24.10) (#24297) Cherry-pick of #24288 (commit d9290f8) from main to 24.10. ## Summary Same fix as #24288: switches pnpm install to hoisted (flat npm-style) layout so transitive npm deps (`@babel/types`, `@lit/reactive-element`, `cookie`, `set-cookie-parser`, `@preact/signals-react/runtime`) are always reachable from project root, eliminating the `vite-basics` IT hang and similar symptoms. ## Adjustments for 24.10 24.10 is pre-`flow-build-tools` refactor (#23161), so all the Java files the original commit touches (`FrontendTools.java`, `FrontendToolsTest.java`, `NodeUpdaterTest.java`, `TaskRunPnpmInstallTest.java`) live under `flow-server/` here. The edits are otherwise identical. Two version adjustments to match this branch's dep graph: 1. **`@babel/types` pinned to `7.28.5`** (matching `@babel/preset-react`) instead of main's `7.29.0`. 24.10 does not declare `@babel/core`, so version coherence is anchored to `preset-react`. 2. **Skipped `@babel/core` and `@babel/plugin-transform-react-jsx-development`** declarations from the main commit — not declared as direct deps on 24.10, and the React function location plugin only imports `@babel/types`. ## Supersedes #24289 This PR replaces #24289 (the narrower @babel/types-only fix). Once this lands, #24289 should be closed. ## Test plan - [ ] CI on this PR passes (specifically `it-tests (1)` and `it-tests (2)`). - [ ] `mvn -pl flow-server test -Dtest=NodeUpdaterTest,TaskRunPnpmInstallTest,FrontendToolsTest` passes. - [ ] After install in any vite-using IT module, `node_modules/.pnpm/` does not exist (or is empty); `node_modules/@babel/types/` and `node_modules/@lit/reactive-element/` exist as real directories at the project root. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Zhe Sun <zhe@vaadin.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ZheSun88
added a commit
that referenced
this pull request
May 8, 2026
… (CP: 24.9) (#24298) Cherry-pick of #24288 (commit d9290f8) from main to 24.9. ## Summary Same fix as #24288: switches pnpm install to hoisted (flat npm-style) layout so transitive npm deps (`@babel/types`, `@lit/reactive-element`, `cookie`, `set-cookie-parser`, `@preact/signals-react/runtime`) are always reachable from project root, eliminating the `vite-basics` IT hang and similar symptoms. ## Adjustments for 24.9 24.9 is pre-`flow-build-tools` refactor (#23161), so all the Java files the original commit touches (`FrontendTools.java`, `FrontendToolsTest.java`, `NodeUpdaterTest.java`, `TaskRunPnpmInstallTest.java`) live under `flow-server/` here. The edits are otherwise identical. Two version adjustments to match this branch's dep graph: 1. **`@babel/types` pinned to `7.27.1`** (matching `@babel/preset-react`) instead of main's `7.29.0`. 24.9 does not declare `@babel/core`, so version coherence is anchored to `preset-react` — which on 24.9 is on the older `7.27.x` line. 2. **Skipped `@babel/core` and `@babel/plugin-transform-react-jsx-development`** declarations from the main commit — not declared as direct deps on 24.9, and the React function location plugin only imports `@babel/types`. ## Test plan - [ ] CI on this PR passes (specifically `it-tests (1)` and `it-tests (2)`). - [ ] `mvn -pl flow-server test -Dtest=NodeUpdaterTest,TaskRunPnpmInstallTest,FrontendToolsTest` passes. - [ ] After install in any vite-using IT module, `node_modules/.pnpm/` does not exist (or is empty); `node_modules/@babel/types/` and `node_modules/@lit/reactive-element/` exist as real directories at the project root. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Zhe Sun <zhe@vaadin.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
The Vite dev server intermittently fails (or hangs) on
vite-basicsandother Vite-using IT modules because transitive npm dependencies are not
consistently reachable from the project's root
node_modules. Seleniumtests then wait for pages that never render, and the matrix shard times
out at the 30-minute job cap.
Two distinct symptoms observed:
Cannot find package '@babel/types'— the bundled Vite pluginreact-function-location-plugin.jsdoesimport * as t from '@babel/types';, but@babel/typeswas onlypresent transitively via
@babel/coreand pnpm did not always hoistit to the root.
Failed to resolve import "@lit/reactive-element/...","cookie","set-cookie-parser","@preact/signals-react/runtime"—all transitives of declared deps (
lit,react-router,@preact/signals-react-transform) — also not consistently hoisted.Both are the same underlying problem: pnpm with
shamefully-hoist=truedoes not deterministically expose every transitive at the project root.
Fix
Three commits, layered:
1. Declare
@babel/typesdirectlyAdds
"@babel/types": "7.29.0"(matching@babel/core) to the bundleddevDependenciesinflow-server/.../dependencies/vite/package.json. Correct on its ownmerits — the plugin imports the package, so the package should be a
real dependency. Updates
NodeUpdaterTest's expected-set accordingly.2. Switch the bundled
.npmrctemplate tonode-linker=hoistedAdds
node-linker=hoistedtoflow-server/src/main/resources/npmrc,the template
TaskRunNpmInstallwrites to the project. This tellspnpm to install in flat npm-style mode — every package (declared
or transitive) at the project root, no
.pnpm/virtual store.Resolutions become deterministic.
shamefully-hoist=trueis left in the template alongside, so manualpnpm installinvocations from a developer shell still pick up atleast the legacy partial-hoist behavior if some user-level config
overrides our
node-linker.3. Pass
--config.node-linker=hoistedon the pnpm CLIThe
.npmrcchange alone did not actually flip pnpm's install layoutin CI:
node_modules/.pnpm/...paths kept appearing in errormessages, indicating pnpm was still using its default
isolatedmode. The pre-existing
--shamefully-hoist=trueCLI flag added byFrontendTools.getPnpmExecutable()likely took precedence and lockedpnpm to the isolated layout (where
shamefully-hoistmakes sense asa partial-hoist heuristic).
Replace
--shamefully-hoist=truewith--config.node-linker=hoistedon the CLI. CLI >
.npmrc> defaults in pnpm's config precedence, sothe install is now unambiguously hoisted regardless of any project-
or user-level
.npmrccontent.FrontendToolsTestupdated to assertthe new flag.
Trade-offs
a few seconds slower than the symlink store for fresh installs.
Acceptable for the determinism gain.
other package, declared or not. Flow's frontend code already relies
on this (e.g. plugins importing transitives), so this is the
current de-facto behavior — just made deterministic.
.npmrcthat lacks the auto-gen marker keeps their config (perexisting
TaskRunNpmInstall.createNpmRcFile()logic), but the newCLI flag still applies — they'll silently get hoisted mode for
Flow-driven installs. Worth flagging in release notes.