Skip to content

install: warn when a link: dependency declares peerDependencies - #35602

Open
robobun wants to merge 9 commits into
mainfrom
farm/b1695740/link-peer-deps-warning
Open

install: warn when a link: dependency declares peerDependencies#35602
robobun wants to merge 9 commits into
mainfrom
farm/b1695740/link-peer-deps-warning

Conversation

@robobun

@robobun robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

What

bun link <name> (and bun install with a link: dependency) now prints a warning when the linked package declares peerDependencies that are not already present in the linked package's own node_modules:

$ bun link my-linked-lib
bun link v1.4.0
warn: Linked package "my-linked-lib" declares peerDependencies that may not resolve from this project:
  - react@^18.0.0
  - react-dom@^18.0.0
  Linked packages resolve modules from their real location on disk. Install these peers in the linked package's own node_modules.

installed my-linked-lib@link:my-linked-lib

Peers marked optional in peerDependenciesMeta are annotated (optional). Peers already installed under the linked package are skipped, so running bun install inside the linked package silences the warning. --silent suppresses it entirely. Install behavior is unchanged.

Why

A link: dependency is a symlink into the package's real source tree. Both bun and Node realpath that symlink before walking node_modules, so a peer installed in the consumer's tree is invisible to the linked package unless the linked package also has it in its own node_modules. That is inherent to symlink-based linking; npm and pnpm have the same limitation with their link commands, and pnpm prints an equivalent warning.

Before this change bun said nothing: Features::LINK has peer_dependencies: false, so the linked package's peers were never read. Users only discovered the problem at runtime as Cannot find package 'react' from '/real/path/to/linked/index.js' (or, worse, as two copies of the peer being loaded).

This does not make the linked package magically find the consumer's peers; doing that would require either a resolver change or a new install option. The warning surfaces the situation and the universally-safe remedy (install the peer under the linked package). --preserve-symlinks is another option for hoisted installs but is not mentioned in the warning because it breaks the isolated linker's store layout and the effective linker is not known at the point the warning is emitted.

Refs #13676.

How

parse_with_json_impl already has the parsed package.json and the Features it was called with. When that is Features::LINK (only the link: resolver passes this), the new warn_linked_peer_dependencies helper walks peerDependencies, skips entries that already exist under the linked package's node_modules, annotates optional peers from peerDependenciesMeta, and prints the warning. The check runs once per linked package per install (the folder-resolver result is cached).

Test

test/cli/install/bun-link.test.ts gains a case covering:

  • bun link <name> prints the warning once with each unresolved peer listed and the real-location explanation
  • a peer already present in the linked package's node_modules is not listed
  • peerDependenciesMeta optional entries are annotated
  • bun install with a link: dependency in package.json prints the same warning
  • no warning when every peer is already installed under the linked package

A package installed via `bun link <name>` (or a `link:` dependency) is a
symlink into its real source tree. Node and bun both realpath that symlink
before walking node_modules, so peers installed in the consumer project are
invisible to the linked package unless it has them in its own node_modules.

npm and pnpm have the same limitation. pnpm prints a warning listing the
peers and explaining why; bun previously said nothing, leaving users to
discover the resolution failure at runtime.

Print a matching warning that lists each peer dependency and points at
`--preserve-symlinks`, which makes the linked package resolve from the
consumer's node_modules. The warning is suppressed under --silent and does
not change install behavior.

Fixes #13676
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Linked packages declaring peerDependencies now produce warnings listing required and optional peers, with guidance to use --preserve-symlinks. Tests cover bun link, bun install with link: dependencies, and silent-mode suppression.

Linked package peer dependency warnings

Layer / File(s) Summary
Symlink peer dependency warning
src/install/lockfile/Package.rs
Adds diagnostic output for peer dependencies declared by linked packages, including optional markers and --preserve-symlinks guidance.
CLI warning and silence coverage
test/cli/install/bun-link.test.ts
Verifies warning output during linking and installation, and confirms --silent suppresses it.

Possibly related PRs

  • oven-sh/bun#35461: Both changes extend symlink handling in Package.rs, including behavior around linked package parsing.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR addresses #13676 by warning on linked packages with peerDependencies and directing users to --preserve-symlinks.
Out of Scope Changes check ✅ Passed The changes stay focused on link peer-dependency warnings and the matching test, with no clear unrelated additions.
Title check ✅ Passed The title is concise and accurately summarizes the main change: warnings for linked packages with peerDependencies.
Description check ✅ Passed The description covers what, why, how, and verification, matching the required template structure closely.

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

@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review at 490dc6e. All review feedback addressed; every review thread is resolved.

The new test in test/cli/install/bun-link.test.ts fails on main (no warning printed) and passes with this change, both locally under debug ASAN and on every CI lane that ran it. The remaining CI reds across builds 80478/80907/81070/81221 are the usual unrelated flaky tests (no-orphans, cpu-prof, complex-workspace, in-process-cron, fetch-leak, etc.) and do not touch bun link or src/install/lockfile/Package.rs.

@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:13 PM PT - Jul 25th, 2026

@robobun, your commit 490dc6e has some failures in Build #81221 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 35602

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

bun-35602 --bun

Comment thread src/install/lockfile/Package.rs Outdated
Reads peerDependenciesMeta so entries marked optional are shown as
"name@version (optional)". The warning still fires when every peer is
optional (matching pnpm), since the realpath behavior applies to optional
peers too when the consumer does install them.
Comment thread src/install/lockfile/Package.rs Outdated
Comment thread src/install/lockfile/Package.rs Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/cli/install/bun-link.test.ts`:
- Around line 503-514: Update the subprocess assertions in the shown `bun link`
test and the corresponding cases around the referenced ranges to drain stdout,
stderr, and exited concurrently rather than awaiting them sequentially. Start
all three promises before awaiting, then assert the captured combined results
while preserving the existing success and error expectations.
- Around line 527-532: Strengthen the warning assertions around stderrForInstall
in both the bun link and bun install cases. Verify the linked-package warning
header occurs exactly once, and assert the warning includes the
real-disk-location explanation alongside the existing peer dependency and
--preserve-symlinks checks. Keep the existing optional and non-optional
dependency assertions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 152a0582-77d9-4708-a293-204706371128

📥 Commits

Reviewing files that changed from the base of the PR and between df6c7ee and 1aa41ed.

📒 Files selected for processing (2)
  • src/install/lockfile/Package.rs
  • test/cli/install/bun-link.test.ts

Comment thread test/cli/install/bun-link.test.ts Outdated
Comment thread test/cli/install/bun-link.test.ts Outdated

@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.

Thanks for addressing the peerDependenciesMeta feedback — optional peers are now annotated and the comment is condensed. I didn't find any correctness issues in this revision, but since this adds new user-facing warning copy to bun install/bun link, a maintainer should sign off on the wording and on whether to still warn when every peer is optional.

What was reviewed:

  • FEATURES == Features::LINK gate — Features derives PartialEq and only folder_resolver.rs passes LINK, so this fires exactly on link: deps
  • peerDependenciesMeta optional lookup matches the existing parse at line ~2222; log_level != Silent matches the pattern used across src/install/
  • --preserve-symlinks is a real resolver flag, so the remedy is accurate
  • Test covers register/link/install/--silent and the (optional) annotation; sequential pipe reads match every other test in this file (output is tiny)
Extended reasoning...

Overview

Adds a ~38-line advisory block in parse_with_json_impl (src/install/lockfile/Package.rs) that fires when parsing a link: dependency's package.json with a non-empty peerDependencies object. It prints a bun_core::warn! header, one line per peer (annotated (optional) when peerDependenciesMeta[key].optional === true), and a two-line hint pointing at --preserve-symlinks. Suppressed under --silent. A new test in test/cli/install/bun-link.test.ts covers all four flows.

Security risks

None. Read-only inspection of already-parsed JSON; output goes to stderr; no change to resolution, lockfile, or exit codes.

Level of scrutiny

Medium. The Rust logic is straightforward and mirrors existing patterns (bun_core::warn!/pretty_errorln!/Output::flush() are used identically in migration.rs and extract_tarball.rs; the log_level != Silent gate appears throughout src/install/; the EBoolean match copies the block at line ~2228). But it introduces new user-facing CLI output on a very common command, and REVIEW.md treats warning/error copy as reviewed word-for-word — the exact phrasing, whether to suppress when all peers are optional, and whether --preserve-symlinks is the recommendation Bun wants to lead with are UX calls a maintainer should make.

Other factors

  • My earlier inline comment about ignoring peerDependenciesMeta is addressed (commit 24483cb2) and the thread is resolved. The comment-cop bot's paragraph-comment complaint is addressed (commit 1aa41ed8).
  • CodeRabbit's two open nits (concurrent pipe drain, stronger assertions) are minor: the new test copies the exact spawn/await shape used by every other test in bun-link.test.ts, and bun link output is a handful of lines so pipe backpressure isn't a real risk here.
  • robobun confirmed the test fails on main and passes with the change, so the test is load-bearing.

…re remedy, soften wording

- Only list peers that are NOT already present in the linked package's own
  node_modules, so the common case (bun install was run in the linked package)
  no longer warns at all.
- Soften "will not resolve" to "may not resolve".
- Under --linker isolated, do not recommend --preserve-symlinks (it breaks the
  isolated store layout); suggest installing peers in the linked package
  instead. Hoisted/auto keeps the --preserve-symlinks hint.
- Pull the warning into a #[cold] helper so the hot parse path stays compact.
- Test now covers: peer already installed (skipped), optional annotation,
  isolated linker remedy text, one warning per link, --silent, bun install.
Comment thread src/install/lockfile/Package.rs Outdated
Comment thread src/install/lockfile/Package.rs
robobun and others added 2 commits July 25, 2026 14:09
…lly safe

- bun_sys::exists_z takes the &ZStr directly, avoiding a PathBuffer copy.
- The effective linker (Auto -> Hoisted/Isolated) is not decided until
  install time, so branching on pm.options.node_linker reads Auto for the
  common workspace-default case. --preserve-symlinks breaks the isolated
  store layout, so rather than risk recommending it there, keep the remedy
  to the universally-correct "install in the linked package's own
  node_modules".
- Test now also covers the case where every peer is installed under the
  linked package (no warning).
Comment thread test/cli/install/bun-link.test.ts Outdated
Matches the cleanup convention used by the sibling tests in this file so a
persistent runner does not accumulate dangling global-link symlinks.
Comment thread test/cli/install/bun-link.test.ts Outdated
The register step creates the global symlink; keep it inside the try so the
finally-unlink still runs if one of its assertions throws. Drop the --silent
sub-case to keep the test under the default timeout on debug builds (the
log_level != Silent guard is a one-line comparison).
Comment thread test/cli/install/bun-link.test.ts
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