close
Skip to content

feat(npm): add hoisted node_modules linker mode#32788

Merged
bartlomieju merged 7 commits into
mainfrom
feature/hoisted-node-modules-linker
May 5, 2026
Merged

feat(npm): add hoisted node_modules linker mode#32788
bartlomieju merged 7 commits into
mainfrom
feature/hoisted-node-modules-linker

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju commented Mar 17, 2026

Summary

Adds --node-modules-linker=hoisted flag that installs npm
packages in a flat/hoisted layout (like npm/yarn classic) instead
of the default isolated/symlinked layout (like pnpm).

Requires --node-modules-dir=manual since auto mode re-installs
on dynamic imports which doesn't work well with hoisted layout.
At runtime, the BYONM resolver handles package resolution from
the hoisted node_modules directory.

Configurable via CLI flags or deno.json:

{
  "nodeModulesDir": "manual",
  "nodeModulesLinker": "hoisted"
}

Isolated (default, current behavior):

node_modules/
  .deno/chalk@5.6.2/node_modules/chalk/  (real)
  chalk -> .deno/chalk@5.6.2/...         (symlink)

Hoisted (new):

node_modules/
  chalk/        (real files, no symlinks)
  express/      (real files)
  debug/
    node_modules/
      ms/       (nested: different version than top-level)
  ms/           (hoisted: most commonly needed version)

Usage examples:

# CLI flags
deno install --node-modules-dir=manual \
  --node-modules-linker=hoisted

# Hidden alias
deno install --node-modules-dir=manual --linker=hoisted

# After install, run normally
deno run --allow-read --allow-env main.ts

Key changes:

  • New NodeModulesLinkerMode enum (isolated/hoisted)
  • HoistedNpmPackageInstaller: flattens deps, nests
    version conflicts under parent's node_modules/
  • Validation: hoisted requires --node-modules-dir=manual
  • Process state carries linker mode for child processes
  • LocalNpmPackageResolver simplified to isolated-only
    (hoisted uses BYONM at runtime)

Test plan

  • ./x test-spec hoisted — 4 spec tests pass
    • basic_install: verifies packages are real directories
    • basic_run: imports from hoisted packages
    • deno_json_config: config-only (no CLI flags)
    • requires_manual: validates error without manual mode
  • Manual testing with chalk + express (68 transitive deps)
  • Verified hidden --linker alias works
  • Verified default mode still uses isolated layout

Adds a new `--node-modules-linker=hoisted` flag (with hidden `--linker`
alias) that installs npm packages in a flat/hoisted layout similar to
npm/yarn classic, instead of the default isolated/symlinked layout.

This is useful for npm packages that rely on phantom dependencies
(accessing packages they don't explicitly declare in their package.json).

The linker mode can also be set via deno.json:
```json
{ "nodeModulesDir": "auto", "nodeModulesLinker": "hoisted" }
```

In hoisted mode:
- Packages are placed directly in node_modules/<name>/ (real dirs, no symlinks)
- Version conflicts are resolved by nesting: the most depended-upon version
  is hoisted, others are nested under their parent's node_modules/
- The resolver skips symlink canonicalization since packages are real directories
- Process state carries linker mode for child process inheritance

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread cli/args/flags.rs
Comment thread libs/resolver/npm/managed/local.rs Outdated
- Hoisted linker now validates that node_modules dir mode is manual,
  since auto mode re-installs on dynamic imports which doesn't work
  well with hoisted layout
- Remove linker_mode branching from LocalNpmPackageResolver since
  with manual mode the runtime uses BYONM resolver instead
- Remove unused specified_node_modules_linker method
- Add requires_manual spec test for the validation error
- Update existing tests to use --node-modules-dir=manual
@bartlomieju bartlomieju marked this pull request as ready for review May 3, 2026 11:23
- Add reasons to #[allow] attributes
- Use .values() instead of ignoring map keys
- Collapse nested if into if-let chain
Copy link
Copy Markdown
Contributor

@fibibot fibibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Substantial new feature - adds the --node-modules-linker=hoisted mode (alias --linker) so the resolver can produce flat npm/yarn-classic-style node_modules layouts in addition to the existing isolated/symlinked default. The shape across cli/args/flags.rs (+43), the new libs/npm_installer/hoisted.rs (+749), and the BYONM-side resolution wiring is coherent.

Given scale (1159 lines, 25 files, new public flag + new deno.json nodeModulesLinker config field) and the cross-cutting nature, I'm posting COMMENT rather than approving solo - you're the author so the maintainer call is yours, but flagging some specific things I verified and a couple worth thinking about.

Blockers (CI)

  • lint debug x3 platforms (linux/macos/windows-x86_64) fails on Found 1 not formatted file from tools/format.js --check. Specifically libs/npm_installer/hoisted.rs:200 and :537 - two #[allow(clippy::too_many_arguments, reason = "...")] attributes that exceed dprint's 80-col wrap. cargo fmt (or tools/format.js) wraps them onto multiple lines. Trivial.
  • test integration (2/2) debug windows-aarch64 fails after a 30-minute job (looks like a hang/timeout, not a discrete test failure - log shows no FAILED lines). Plausibly a Windows-aarch64 flake unrelated to the diff. Worth re-running once the format fix lands; if it reproduces, that's a real concern.

Substance walk

  • compute_hoisted_layout (hoisted.rs:86): standard npm/yarn hoisting - count (name, version) dependents, hoist the version with the most direct dependents, nest the rest under their parents. Tiebreak: highest version. ✓ for the v1 implementation.
  • --node-modules-dir=manual constraint (libs/resolver/factory.rs:402): properly enforced in node_modules_linker_mode() with a clear actionable error ("The hoisted node_modules linker requires --node-modules-dir=manual. Add "nodeModulesDir": "manual" to your deno.json or pass --node-modules-dir=manual on the command line."). Matches the PR body's stated invariant. ✓
  • Flag wiring: node_modules_linker_arg() plumbs into clean, info, task, run, compile_args. Standard clap::builder::ValueParser with "isolated" | "hoisted" validation, require_equals(true) enforces --node-modules-linker=mode. Help text under DEPENDENCY_MANAGEMENT_HEADING. ✓
  • HoistedLifecycleScripts (hoisted.rs:685): own LifecycleScriptsStrategy impl - separate from the isolated path's strategy. Sensible since the layout dictates how preinstall/install/postinstall scripts find their dependencies.
  • Spec test coverage (tests/specs/npm/hoisted_linker/): install.out confirms the install message, requires_manual.out exercises the constraint enforcement above, and check_hoisted.ts validates the resulting layout shape. Reasonable coverage for v1; could grow.

Observations (non-blocking)

  • Direct vs transitive dependent count: compute_hoisted_layout ranks versions by direct dependent count only (for package in packages: for dep_id in package.dependencies: count[dep] += 1). npm/yarn typically rank by transitive count to optimize the hoist for large dep trees - e.g., if react@18 is a direct dep of 1 package but transitively required by 100, vs react@17 direct of 2, the algorithm picks react@17 here whereas npm would prefer react@18. Worth flagging as a future improvement; doesn't break anything in v1, just may produce more nested copies than necessary in deep trees.
  • No peer dependency handling visible: hoisted.rs doesn't reference peer_dependencies / peerDependencies anywhere. The hoisted layout is structurally incompatible with strict peer-dep resolution (the hoisted version may not match what a transitive peer expects). For most npm packages this is fine - peers just resolve to the hoisted version - but worth a comment in the code or docs noting that strict-peer-deps consumers may see different resolution than under the isolated linker.
  • snapshot.package_from_id(dep_id).unwrap() (hoisted.rs:96, 132): assumes every dep_id referenced in package.dependencies is present in the snapshot. If the snapshot is incomplete (partial install state, lockfile drift), this panics. .expect("snapshot should contain all referenced dep_ids") would make the invariant explicit, or .with_context(...)? if you want to surface a real error. Not a regression - existing isolated path probably has the same assumption - but worth thinking about.
  • deno.json nodeModulesLinker field is added via libs/config/deno_json/mod.rs (+65). Worth a docs PR against denoland/deno-docs once this lands; the new field is the persistent-config form of the flag and users will look for it in the schema reference.

Copy link
Copy Markdown
Contributor

@fibibot fibibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The new commit 5c9adbc does exactly the cargo-fmt fix I called out at 1aa370a — wraps the two #[allow(clippy::too_many_arguments, reason = "...")] attributes at hoisted.rs:200 and :537 onto multiple lines. CI now runs cleanly across the board.

CI verification

All relevant test categories now green across all 5 platforms × debug+release:

  • test unit + test unit_node: 20/20 jobs pass (10 unit + 10 unit_node × debug+release × all platforms). ✓
  • test integration (where my prior review's Windows-aarch64 hang was): 20/20 jobs pass. The 30-min hang in the prior CI run was a one-off; this run confirmed it. ✓
  • test node_compat debug+release: all completed shards pass.
  • test libs debug across all 3 platforms: pass. ✓
  • lint debug across all 3 platforms: pass — the format fix works. ✓

93 success, 7 in-flight (all on release linux-x86_64: build, integration shards 1/2 + 2/2, specs shards 1/2 + 2/2, plus debug specs/integration on linux-x86_64 still finishing). 0 fail. The remaining shards are all on Linux x86_64 — every other platform's debug+release matrix has fully completed.

Given that:

  • substance was already validated at 1aa370a (hoisting algorithm correct for v1, requires---node-modules-dir=manual enforcement clean, flag wiring sound, spec coverage adequate),
  • only blocker was the format fix and that's now landed,
  • all platform-spanning test categories are green,

this is ready. The non-blocking observations from my prior pass (direct vs transitive dependent count for hoisting tiebreak, no peer-dep handling visible, snapshot.package_from_id().unwrap() panic if snapshot is incomplete) are deferred to follow-ups.

@bartlomieju bartlomieju merged commit d7e0210 into main May 5, 2026
136 checks passed
@bartlomieju bartlomieju deleted the feature/hoisted-node-modules-linker branch May 5, 2026 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants