Skip to content

install: accept installConfig.hoistingLimits "none" without a warning - #41240

Merged
Jarred-Sumner merged 1 commit into
mainfrom
robobun/a58aecde/hoisting-limits-none
Sep 3, 2026
Merged

Jarred-Sumner merged 1 commit into
mainfrom
robobun/a58aecde/hoisting-limits-none

Conversation

@robobun

@robobun robobun commented Sep 3, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • bun install warns for each workspace whose package.json has "installConfig": { "hoistingLimits": "none" }:
    warn: workspace "app": installConfig.hoistingLimits "none" is not supported (only "workspaces" is); ignoring
    Bun 1.4.0 prints nothing. install: self-contained workspaces for the hoisted linker (workspaces.selfContained / installConfig.hoistingLimits) #40014 added the warning, and no release has it yet.
  • process_workspace_name (src/install/lockfile/Package/WorkspaceMap.rs:203) flags every value other than "workspaces". But "none" is the default value in Yarn. It sets no hoisting limit, and that is how bun hoists every workspace.

Fix

  • Bun now handles "none" like a missing key. It prints no warning, and the workspace is not self-contained. The root workspaces.selfContained list still applies, as it does for a missing key.
  • "dependencies" keeps the warning, because bun does not support it. The warning text now names both accepted values.
  • Yarn reads the key as installConfig?.hoistingLimits ?? nmHoistingLimits, and the default of nmHoistingLimits is none. Only workspaces and dependencies make a hoisting border (buildNodeModulesTree.ts in @yarnpkg/nm).
  • Verified: test/cli/install/bun-workspaces-self-contained.test.ts (2 new cases, both fail on main) and test/cli/install/bun-workspaces.test.ts. Self-reviewed: 2 concerns raised, 1 addressed. The notes explain the other.

Background

Notes

Repro (canary 1.4.1-canary.1+a6c4cc276, then a build of this branch):

mkdir -p r/packages/app && cd r
echo '{"name":"r","private":true,"workspaces":["packages/*"]}' > package.json
echo '{"name":"app","version":"1.0.0","installConfig":{"hoistingLimits":"none"}}' > packages/app/package.json
bun install

Before: the warning above, exit 0. After: no warning, exit 0. With "dependencies", the warning stays:

warn: workspace "app": installConfig.hoistingLimits "dependencies" is not supported (only "workspaces" and "none" are); ignoring

Tests

  • hoistingLimits "none" hoists the workspace normally and does not warn: no warning, no apps/desktop/node_modules, and bun.lock records no hoistingLimits.
  • A new describe.each row, "none" plus the root selfContained list: no warning, and the workspace is self-contained across a frozen install and a reinstall.
  • Both fail with the canary build and pass with this branch. The 6 other cases in the file pass with both builds.

Self-review

  • Addressed: the docs sentence was long. It now says that only "workspaces" changes the layout.
  • Not changed: the root selfContained list wins over a manifest "none". In Yarn, a manifest value wins over the project-wide nmHoistingLimits. The bun list is not a project-wide default. It names each workspace by its exact path or name. The list already wins over "dependencies" and over a missing key.

Other PRs


no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-workspaces-self-contained.test.ts

"none" is the default value of installConfig.hoistingLimits in Yarn. It
means that the workspace has no hoisting limit, which is how bun hoists
every workspace. bun install printed "is not supported" for each
workspace that has this value. It now treats "none" like a missing key.
The warning stays for "dependencies", which bun does not support.
@robobun

robobun commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fixed in this PR. CI runs next.

Reproduction: the canary build 1.4.1-canary.1+a6c4cc276 prints the warning on bun install for a workspace with "installConfig": { "hoistingLimits": "none" }. The PR body notes have the commands.

Proof: the 2 new cases in test/cli/install/bun-workspaces-self-contained.test.ts fail with the canary build. They pass with a debug build of this branch.

@github-actions github-actions Bot added the claude label Sep 3, 2026
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 18 days. After that, they cost $0.25 per reviewed file.

Or wait 32 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 9d9fbbc5-520c-4c0b-b939-ad51a22fdea1

📥 Commits

Reviewing files that changed from the base of the PR and between 467e741 and daa9d15.

📒 Files selected for processing (4)
  • docs/pm/workspaces.mdx
  • src/install/lockfile/Package.rs
  • src/install/lockfile/Package/WorkspaceMap.rs
  • test/cli/install/bun-workspaces-self-contained.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Sep 3, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:57 PM PT - Sep 2nd, 2026

✅ @robobun, your commit daa9d15edc55252829a1ccf2f38c04eea7eb2eaf passed in Build #109596! 🎉


🧪   To try this PR locally:

bunx bun-pr 41240

That installs a local version of the PR into your bun-41240 executable, so you can run:

bun-41240 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — small, well-scoped fix that stops warning on Yarn's default hoistingLimits: "none".

What was reviewed:

  • WorkspaceMap.rs: hoisting_limits bool stays false for "none" (only "workspaces" sets it), and "none" is now excluded from unsupported_hoisting_limits — so it's a true no-op, matching Yarn's default semantics.
  • Warning text in Package.rs and the docs wording update are consistent with the new behavior.
  • New tests cover "none" alone (hoists normally, no warning, nothing in lockfile) and "none" + root selfContained (root list still wins, no warning); the existing warning assertion is tightened to also assert absence of the warning for non-"dependencies" cases.
Extended reasoning...

Overview

This PR touches four files: a one-line guard change in src/install/lockfile/Package/WorkspaceMap.rs to treat installConfig.hoistingLimits: "none" as a recognized no-op (excluding it from unsupported_hoisting_limits alongside "workspaces"), a warning-message text update in src/install/lockfile/Package.rs, a one-word docs wording tweak in docs/pm/workspaces.mdx, and two new test cases plus a tightened assertion in test/cli/install/bun-workspaces-self-contained.test.ts. The core logic change is a single matches! pattern.

Security risks

None. This only affects whether a diagnostic warning is emitted for a specific string value read from a workspace's own package.json. No path handling, no network, no allocation sizing, no privilege boundaries are touched.

Level of scrutiny

Low. The semantic question is narrow: does "none" correctly become a no-op? I verified that hoisting_limits: bool (line 202) is still computed as == Some(b"workspaces"), so "none" does not mark the workspace self-contained — it only suppresses the warning, which is exactly what the PR claims. The root selfContained list behavior is unchanged since it's driven by the separate hoisting_limits bool and root config, and the new describe.each row exercises that interaction.

Other factors

Tests are added to the existing feature test file (per repo convention), use the existing writeProject/install harness, assert stderr before exit code, and add a negative assertion (not.toContain("hoistingLimits")) to the else branch so the non-warning cases are actively checked rather than vacuous. No CODEOWNERS entries cover these paths. The bug hunt exited on dry_streak with no findings and no ruled-out candidates. The PR timeline shows no prior reviews or outstanding objections.

@Jarred-Sumner
Jarred-Sumner merged commit 4b540e6 into main Sep 3, 2026
11 of 12 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the robobun/a58aecde/hoisting-limits-none branch September 3, 2026 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants