feat(pkg-manager): add new hoisting limits config by jakebailey · Pull Request #6468 · pnpm/pnpm

jakebailey · 2023-04-25T23:33:51Z

This adds a new option which behaves the same as yarn's nmHoistingLimits config when pnpm is using node-linker=hoisted.

I have reused the same config names, however, they may require tweaking to reflect pnpm's terminology. I'd love some feedback on that.

It also includes a bugfix needed to get things working; I can split that into another PR.

This PR needs tests, I'm pretty sure. Will work on that.

jakebailey · 2023-04-26T01:55:07Z

I have absolutely no clue what failed in CI...

zkochan · 2023-04-26T14:19:33Z

It was an issue with verdaccio. I have downgraded it. Now you can see the failed test.

jakebailey · 2023-04-26T17:47:22Z

Seems like my bugfix introduces a breaking change to the lockfile. Before, a dependency like "minimist": "" would be encoded like:

dependencies:
  minimist:
    specifier: ''
    version: 1.2.8

But, now, it's encoded like:

dependencies:
  minimist:
    version: 1.2.8

The problem I was papering over was that some random deps got '' as their specifier, which was tripping up headlessInstall and making it throw OUTDATED_LOCKFILE as it believed that the manifest didn't satisfy the wanted lockfile.

I guess I can not do this bugfix, and try and come up with some other fix for that issue, which, of course also deserves a test, if I can figure out how to write one anyway 😅

jakebailey · 2023-04-26T17:49:08Z

I think in my bugfix I was mirroring how addLinkToLockfile ignores '', but maybe the fix is just to still only do this for linked deps. But, I'll try and write a proper test instead.

jakebailey · 2023-05-11T17:41:54Z

  manifest: Pick<ProjectManifest, DependenciesOrPeersField>,
  depName: string
-) {
+): string | undefined {


This is technically now breaking but not sure I can do better here. I'm failing to figure out what the deal is, effectively what's happening is that a package depends on ts-node, which peer deps on @types/node, which exists in the repo and should be linked, but that peer dep appears as a dep of the user of ts-node instead.

is it? looks like CI is passing

Breaking API change, I mean, in that this used to return string but now returns string | undefined.

But, I searched github and found nobody but pnpm calling this function so I don't expect it to acutally matter...

I do think that this change is good, but, I also think I'm masking another bug regarding peer deps (but I am at least getting DT to work after this).

I guess if I'm making the other major bumps, I may as well declare this one as major too?

Ah, no, I already got that in the PR.

zkochan · 2023-05-11T22:39:55Z

+ */
+export type HoistingLimitMode = 'none' | 'workspaces' | 'dependencies'
+
+export function getHoistingLimits (lockfile: Lockfile, mode: HoistingLimitMode | undefined): HoistingLimits | undefined {


there is no need to make this function public. Just call it from the hoist function instead of passing hoistingLimits to the hoist function.

I guess I can shove it down the stack; the main thing I was trying to reconcile is the fact that this API takes in hoisting limits and so I need to respect them. But, I guess I can just do this somewhere else.

There's no need to take in hoisting limits. It is not used anywhere.

I'm not following; are you saying that #4300 can in effect be reverted? That is the API I'm referring to, and it's exposed all the way up to headlessInstall.

yes, I needed it for Bit but then I came up with a different solution. So, it is not needed.

I'm happy to revert that API and make this logic private, if you're confident that it's safe to do so! I'm not totally sure what API guarantees you make, so I went with the conservative option.

It is OK. It will be a major version bump.

jakebailey · 2023-05-11T23:04:00Z

One thing I'm struggling with is producing a test for the bugfix; it's going to require adding a new package to the fake registry in pkg-manager/core, but I'm not sure how to do that.

In effect, I need to have these packages:

// packages/a/package.json
{
    "name": "a"
}
// packages/b/package.json
{
    "name": "b",
    "devDependencies": {
        "external": "*"
    }
}
// On npm, not declared locally
{
    "name": "external"
    "peerDependencies": {
        "a": "*"
    }
}

I can try and find real npm packages out there that could do this, but most have a lot of other dependencies that would make pulling in the full tree awkward.

zkochan · 2023-05-11T23:10:50Z

The fake registry is here: https://github.com/pnpm/registry-mock

You can link it locally to the pnpm repo or I can publish a new version when you add the packages.

zkochan · 2023-05-11T23:11:55Z

Isn't what you posted a workspace?

// packages/a/package.json
{
    "name": "a"
}
// packages/b/package.json
{
    "name": "b",
    "devDependencies": {
        "external": "*"
    }
}
// On npm, not declared locally
{
    "name": "external"
    "peerDependencies": {
        "a": "*"
    }
}

If it is a workspace these packages shouldn't be in the registry mock.

jakebailey · 2023-05-11T23:14:35Z

Two of them are, but if external is declared locally, then it's a linked local dep and the bug doesn't appear.

I didn't realize the mock was pulling from anything except https://github.com/pnpm/pnpm/tree/main/pkg-manager/core/test/registry-mirror; I'll go check that mock repo to see if it has something I can use!

jakebailey · 2023-05-11T23:27:49Z

+"@pnpm/resolve-dependencies": major
+"@pnpm/manifest-utils": major
+"@pnpm/real-hoist": major
+"@pnpm/headless": major
+"@pnpm/core": major
+"@pnpm/config": major


Was not sure here as the API changed, so... just said they're all major. I'm not sure if that's accurate or not.

…ageManifest

jakebailey · 2023-05-12T00:17:39Z

Was able to get a test for the bugfix in the first commit. Will work on tests for the hoisting limits.

I can also split the bugfix apart if you want that sooner than I can write tests for the second half.

zkochan · 2023-05-12T19:58:13Z

I am ok with whichever approach

zkochan · 2025-05-11T16:04:31Z

is this still needed?

jakebailey · 2025-05-11T16:07:59Z

I unfortunately haven't had time to work on this; doing this would enable DT to stop needing to disable the shared lock file mode and therefore take way less time to install. It's just been "fine" since we switched to pnpm such that other stuff has been more pressing...

AsamK · 2025-05-13T11:35:52Z

I'm also interested in yarn like hoisting limits and would have time to work on it.
What's still missing to get this finished? Just the tests for the new hoisting limits?

For the terminology, it might make sense to rename "workspaces" to "packages" to match the pnpm workspace terminology.
-> 'hoisting-limits': ['none', 'packages', 'dependencies'],

jakebailey · 2025-05-13T18:16:05Z

I'm honetly not 100% certain what all is left at this point; the primary place to test this is on DefiniteltyTyped by enabling this option and seeing if the install is both faster and still passes tests (within reason).

If you have more time than me to work on it, by all means feel free to.

zkochan · 2026-05-28T22:22:21Z

This will be shipped via #12041

Expose `hoistingLimits` as a user-facing setting for `nodeLinker: hoisted` installs, mirroring yarn's `nmHoistingLimits`. Accepts `none` (default — hoist as far as possible), `workspaces` (hoist only as far as each workspace package), or `dependencies` (hoist only up to each workspace package's direct dependencies). The setting is the user-friendly enum; `@pnpm/installing.linking.real-hoist` translates it into the `@yarnpkg/nm` hoister's per-locator border map via `getHoistingLimits`. Wires the key through config types, the config-file key registry, and the install/add/recursive command option lists. Revives the approach from #6468 (closes #6457).

…its setting (#12041) ## 1. Support `nodeLinker: hoisted` on the fresh-lockfile install path (pacquet) Closes #11871. Until now pacquet's `Install::run` hard-refused `nodeLinker: hoisted` without a checked-in lockfile (`ERR_PNPM_…UNSUPPORTED_FRESH_INSTALL_NODE_LINKER`). - Extracted a shared `run_hoisted_linker` helper from the frozen path's hoisted branch (walker → `link_hoisted_modules` → `SymlinkDirectDependencies { link_only: true }` → `pkg_root_by_key` → walker-skip folding), so both install paths run identical logic. - Fresh path now threads `node_linker` + `supported_architectures`, hands `CreateVirtualStore` the real linker (populating `cas_paths_by_pkg_id`), branches on `is_hoisted`, and returns `hoisted_locations` so `.modules.yaml` round-trips. - Removed the guard and the dead `UnsupportedFreshInstallNodeLinker` error variant. Ported upstream's `hoistedNodeLinker/install.ts` into `crates/cli/tests/hoisted_node_linker.rs` (real tests for the core layout, no-lockfile, `externalDependencies`, `autoInstallPeers`, and `hoistingLimits`; the rest stubbed as `known_failures` against `pnpm add`/update (#433) and build-phase (#11870) gaps), and ticked the boxes in `plans/TEST_PORTING.md`. ## 2. Add the `hoistingLimits` setting (pnpm CLI **and** pacquet) Revives the stale #6468 (closes #6457) and brings both stacks to parity. `hoistingLimits` mirrors yarn's `nmHoistingLimits`: `none` (default — hoist as far as possible), `workspaces` (hoist only as far as each workspace package), `dependencies` (hoist only up to each workspace package's direct deps). It was previously a programmatic-only option in pnpm (no config surface) and a pacquet-only raw-map yaml field. **pnpm CLI:** `config/reader` (`types.ts` enum + `Config.ts` + `configFileKey.ts`), `installing/linking/real-hoist`'s new `getHoistingLimits` (mode → the `@yarnpkg/nm` hoister's per-locator border map), and the install/add/recursive command option lists. Tests: `hoistedNodeLinker/install.ts` (`dependencies` mode) + `real-hoist` `getHoistingLimits` unit tests. Changeset included (minor). **pacquet:** replaced the raw-map config with the same enum; added `get_hoisting_limits` (port of `getHoistingLimits`); and **fixed `real-hoist`'s border semantics** — a name in the limits marks a *border* whose descendants stay nested beneath it, not a leaf to block. (The earlier leaf-blocking behavior was the divergence flagged while porting; its unit tests were rewritten to the corrected semantics.)

zkochan · 2026-05-28T23:49:15Z

Revived and merged in #12041.

Your getHoistingLimits design — the none / workspaces / dependencies enum translating into the @yarnpkg/nm hoister's per-locator border map — landed essentially as proposed here, refreshed for the current repo layout (config/reader, installing/linking/real-hoist, installing/commands), with tests and a matching port in the pacquet Rust implementation. Thanks @jakebailey for the original work — closing this as superseded.

Written by an agent (Claude Code, claude-opus-4-8).

…its setting (#12041) ## 1. Support `nodeLinker: hoisted` on the fresh-lockfile install path (pacquet) Closes #11871. Until now pacquet's `Install::run` hard-refused `nodeLinker: hoisted` without a checked-in lockfile (`ERR_PNPM_…UNSUPPORTED_FRESH_INSTALL_NODE_LINKER`). - Extracted a shared `run_hoisted_linker` helper from the frozen path's hoisted branch (walker → `link_hoisted_modules` → `SymlinkDirectDependencies { link_only: true }` → `pkg_root_by_key` → walker-skip folding), so both install paths run identical logic. - Fresh path now threads `node_linker` + `supported_architectures`, hands `CreateVirtualStore` the real linker (populating `cas_paths_by_pkg_id`), branches on `is_hoisted`, and returns `hoisted_locations` so `.modules.yaml` round-trips. - Removed the guard and the dead `UnsupportedFreshInstallNodeLinker` error variant. Ported upstream's `hoistedNodeLinker/install.ts` into `crates/cli/tests/hoisted_node_linker.rs` (real tests for the core layout, no-lockfile, `externalDependencies`, `autoInstallPeers`, and `hoistingLimits`; the rest stubbed as `known_failures` against `pnpm add`/update (#433) and build-phase (#11870) gaps), and ticked the boxes in `plans/TEST_PORTING.md`. ## 2. Add the `hoistingLimits` setting (pnpm CLI **and** pacquet) Revives the stale #6468 (closes #6457) and brings both stacks to parity. `hoistingLimits` mirrors yarn's `nmHoistingLimits`: `none` (default — hoist as far as possible), `workspaces` (hoist only as far as each workspace package), `dependencies` (hoist only up to each workspace package's direct deps). It was previously a programmatic-only option in pnpm (no config surface) and a pacquet-only raw-map yaml field. **pnpm CLI:** `config/reader` (`types.ts` enum + `Config.ts` + `configFileKey.ts`), `installing/linking/real-hoist`'s new `getHoistingLimits` (mode → the `@yarnpkg/nm` hoister's per-locator border map), and the install/add/recursive command option lists. Tests: `hoistedNodeLinker/install.ts` (`dependencies` mode) + `real-hoist` `getHoistingLimits` unit tests. Changeset included (minor). **pacquet:** replaced the raw-map config with the same enum; added `get_hoisting_limits` (port of `getHoistingLimits`); and **fixed `real-hoist`'s border semantics** — a name in the limits marks a *border* whose descendants stay nested beneath it, not a leaf to block. (The earlier leaf-blocking behavior was the divergence flagged while porting; its unit tests were rewritten to the corrected semantics.)

zkochan · 2026-05-29T15:52:03Z

🚢 11.5

jakebailey changed the title ~~feat(pkg-manager): add new hoist-limit-mode config~~ feat(pkg-manager): add new hoisting-limit-mode config Apr 25, 2023

jakebailey force-pushed the new-hoisting branch from 28ce1ec to 9811e6d Compare April 25, 2023 23:54

jakebailey marked this pull request as ready for review April 25, 2023 23:59

jakebailey requested a review from zkochan as a code owner April 25, 2023 23:59

jakebailey marked this pull request as draft April 26, 2023 02:14

zkochan force-pushed the new-hoisting branch from 9811e6d to 5ad665b Compare April 26, 2023 12:30

jakebailey force-pushed the new-hoisting branch from 5ad665b to e56ad4e Compare May 11, 2023 17:38

jakebailey commented May 11, 2023

View reviewed changes

zkochan reviewed May 11, 2023

View reviewed changes

jakebailey changed the title ~~feat(pkg-manager): add new hoisting-limit-mode config~~ feat(pkg-manager): add new hoisting limits config May 11, 2023

jakebailey commented May 11, 2023

View reviewed changes

Comment thread pkg-manager/headless/src/index.ts Outdated

jakebailey added 3 commits May 11, 2023 17:15

fix(pkg-manager): distinguish empty from undefined in getSpecFromPack…

5245ce6

…ageManifest

feat(pkg-manager): add new hoist-limit-mode config

cbb6941

feat: change hoisting limit API to match yarn's nmHoistingLimits

4778892

jakebailey force-pushed the new-hoisting branch from ec51c19 to 4778892 Compare May 12, 2023 00:16

jakebailey mentioned this pull request May 15, 2023

ERR_PNPM_OUTDATED_LOCKFILE Cannot install with "frozen-lockfile" because pnpm-lock.yaml is not up to date with ... #6526

Closed

zkochan closed this May 28, 2026

zkochan mentioned this pull request May 28, 2026

feat: support nodeLinker: hoisted on fresh installs + add hoistingLimits setting #12041

Merged

zkochan mentioned this pull request May 28, 2026

Shared lockfile results in nondeterministic top-level packages, differing behavior from npm install #6457

Closed

Uh oh!

Conversation

jakebailey commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakebailey commented Apr 26, 2023

Uh oh!

zkochan commented Apr 26, 2023

Uh oh!

jakebailey commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakebailey commented Apr 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zkochan May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey commented May 11, 2023

Uh oh!

zkochan commented May 11, 2023

Uh oh!

zkochan commented May 11, 2023

Uh oh!

jakebailey commented May 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakebailey commented May 12, 2023

Uh oh!

zkochan commented May 12, 2023

Uh oh!

zkochan commented May 11, 2025

Uh oh!

jakebailey commented May 11, 2025

Uh oh!

AsamK commented May 13, 2025

Uh oh!

jakebailey commented May 13, 2025

Uh oh!

zkochan commented May 28, 2026

Uh oh!

zkochan commented May 28, 2026

Uh oh!

zkochan commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jakebailey commented Apr 25, 2023 •

edited

Loading

jakebailey commented Apr 26, 2023 •

edited

Loading

zkochan May 11, 2023 •

edited

Loading