resolver: don't let the main fallback clobber the module entry's sideEffects

[robobun]

What does this PR do?

Fixes #8993.

When a package has both "module" and "main", the resolver records the "main" path as path_pair.secondary so the bundler can fall back to it for require() interop. finalize_result iterates both paths and was writing primary_side_effects_data for each, so the secondary's sideEffects lookup overwrote the primary's.

A bare import "pkg" where the package lists its "module" entry in "sideEffects" (but not the "main" entry) was therefore marked side-effect-free and tree-shaken out of the bundle. This is the common shape for packages that ship a pre-bundled CJS main alongside ESM sources, e.g. @tensorflow/tfjs-backend-cpu:

{
  "name": "@tensorflow/tfjs-backend-cpu",
  "main": "dist/tf-backend-cpu.node.js",
  "module": "dist/index.js",
  "sideEffects": ["./dist/index.js", "./dist/base.js", "./dist/register_all_kernels.js"]
}

Repro

// index.ts
import { tensor } from "@tensorflow/tfjs-core";
import "@tensorflow/tfjs-backend-cpu";
console.log(tensor([Math.random()]).toString());

bun build index.ts --outdir dist --target bun
bun dist/index.js
# TypeError: undefined is not an object (evaluating 'env().platform.isTypedArray')

The existing dce/PackageJsonSideEffectsArrayKeepModule* tests in test/bundler/esbuild/dce.test.ts happen to avoid this because their fixture package.json files omit "name", which prevents the package from being recorded as enclosing_package_json, so result.package_json is None on the second iteration and the overwrite is skipped. Real packages always have "name".

Fix

Compute primary_side_effects_data only from the first (primary) path yielded by path_pair.iter(). The secondary iteration still runs for symlink/tsconfig/module-type handling but no longer touches the side-effects field.

How did you verify your code works?

New tests in test/bundler/esbuild/dce.test.ts (all fail on main, pass with this change):

• dce/PackageJsonSideEffectsArrayModuleMainBareImport: package with name + module + main, sideEffects lists only the module entry; bare import must be kept.
• dce/PackageJsonSideEffectsGlobModuleMainBareImport: same with a glob pattern (matches the @tensorflow/tfjs-core shape).
• dce/PackageJsonSideEffectsArrayModuleMainBareImportRemove: inverse guard; sideEffects lists only the main entry, bare import resolves to module and must still be dropped.

Full dce.test.ts (81 pass / 17 todo / 0 fail) and packagejson.test.ts (78 pass / 9 todo / 0 fail) pass with no regressions. The original tfjs repro now bundles to 570 KB (258 KB minified) and runs correctly.

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/esbuild/dce.test.ts

[robobun]

Status: diff is green; CI red on unrelated lanes only. Ready for a maintainer to merge.

Reproduced with @tensorflow/tfjs-core@4.22.0 + @tensorflow/tfjs-backend-cpu@4.22.0 on linux-x64; bundle crashed at env().platform.isTypedArray. After the fix the bundled output prints the tensor.

Fail-before / pass-after via test/bundler/esbuild/dce.test.ts -t ModuleMainBareImport. Full dce.test.ts (81 pass) and packagejson.test.ts (78 pass) green locally.

CI runs (both builds: only unrelated failures):

• build 86773: test/js/web/timers/setInterval.test.js memory-leak timeout on debian 13 x64-asan; rest flaky.
• build 86919: test/cli/install/bun-upgrade.test.ts on windows-aarch64 ("Canary builds are not available for this platform yet"); rest flaky.

Neither touches the resolver or bundler; both reported to main-break triage.

Re: the bot findings:

• resolver: record enclosing package.json regardless of "name" field #33883 contains the same is_primary guard plus a broader enclosing_package_json change for nameless package.json files. This PR is the narrow slice that fixes Bun.build does not seem to include all imports #8993 on its own; either merging this first or folding it into resolver: record enclosing package.json regardless of "name" field #33883 works.
• import "temporal-polyfill/global" is tree-shaken and not included in the build #22598 (temporal-polyfill/global) no longer reproduces on current main (the exports-map path already keeps the side-effect import), so this PR does not close it.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 62d4fbde-2c17-436a-9586-e4c1d3c33cfb

📥 Commits

Reviewing files that changed from the base of the PR and between 5f7e62d and 6d2172f.

📒 Files selected for processing (2)

• src/resolver/resolver.rs
• test/bundler/esbuild/dce.test.ts

---

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

[robobun]

^{Updated 1:22 AM PT - Aug 1st, 2026}

⏳ @robobun, your commit 6d2172f is still building in Build #86919, but has 1 failures so far (All Failures):

• test/cli/install/bun-upgrade.test.ts - code 1 on 🪟 11 aarch64

[github-actions]

Found 1 issue this PR may fix:

1. import "temporal-polyfill/global" is tree-shaken and not included in the build #22598 - The temporal-polyfill package has both module and main fields plus a sideEffects array; the bare import import 'temporal-polyfill/global' is incorrectly tree-shaken, matching the exact bug pattern this PR fixes (main fallback overwriting the module entry's sideEffects data).

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #22598

🤖 Generated with Claude Code

[github-actions]

This PR may be a duplicate of:

1. resolver: record enclosing package.json regardless of "name" field #33883 - Both PRs add an is_primary guard in finalize_result to prevent the secondary (main) path from clobbering primary_side_effects_data, fixing the same sideEffects tree-shaking bug

🤖 Generated with Claude Code

[claude]

LGTM — narrow, well-tested fix; the only finding is a comment-verbosity nit.

Note: the bot flagged #33883 as a possible duplicate; may want to close/reconcile that one.

Extended reasoning...

Overview

Two files changed. In src/resolver/resolver.rs, finalize_result now gates the two writes to result.primary_side_effects_data on is_primary (true only for the first path yielded by path_pair.iter()), so the CJS-interop main fallback recorded as path_pair.secondary no longer overwrites the module entry's side-effects verdict. The Glob(_) and Mixed(_) match arms in the first block are also merged — their bodies were byte-identical. In test/bundler/esbuild/dce.test.ts, three new itBundled cases cover Map-form sideEffects, glob-form, and the inverse (module NOT listed → still dropped).

Security risks

None. This is bundler tree-shaking metadata; no untrusted-input parsing, syscalls, or auth surface is touched.

Level of scrutiny

Medium. The resolver is a hot/critical path, but the change is a two-line guard whose correctness follows directly from PathPairIter::next_ (index 0 → primary, index 1 → secondary). I verified the iterator yields primary first, so core::mem::replace(&mut is_primary_path, false) is true exactly on the primary iteration. The secondary iteration still runs for tsconfig/module-type/symlink handling — only the side-effects write is skipped, matching the field's name (primary_side_effects_data). If the primary is is_disabled and skipped by the iterator, the secondary is treated as first — same effective behavior as before the PR (where secondary always wrote last), so no regression there.

Other factors

• Tests follow neighboring conventions: todo: isWindows matches the adjacent PackageJsonSideEffectsArrayKeep* tests (path-separator handling in sideEffects arrays is a known Windows todo). The inverse test omits it, which is fine — a non-match on Windows still yields the expected drop.
• The PR description explains why existing PackageJsonSideEffectsArrayKeepModule* tests didn't catch this (fixture package.json lacks "name", so enclosing_package_json is unset and the second iteration's write path is skipped); the new fixtures include "name" and comment that it's load-bearing.
• Fail-before/pass-after was verified per the description (dce.test.ts 81 pass / 0 fail; packagejson.test.ts 78 pass / 0 fail) and the tfjs repro now runs.
• The single inline finding is a REVIEW.md comment-style nit (bug-history narration in the resolver comment and test header). It doesn't affect correctness and can be trimmed at the author's discretion.

[claude]

LGTM — the comment-trim nit from my earlier pass is addressed in 6d2172f, and the is_primary guard is the right fix (the field is literally named primary_side_effects_data).

What was reviewed:

• PathPair::iter() yields primary at index 0 then secondary, so mem::replace(&mut is_primary_path, false) correctly tags only the first iteration; the continue on a failed read_dir_info for the primary still consumes the flag, which is fine (falling back to the secondary's sideEffects would just reintroduce the bug).
• Both write sites of primary_side_effects_data are gated; the collapsed Glob | Mixed arm is byte-identical to the two arms it replaces.
• New tests follow the neighboring PackageJsonSideEffectsArrayKeepModule* shape (including todo: isWindows), and the inverse-guard test keeps the drop path covered.

Extended reasoning...

Overview

Two-file change: src/resolver/resolver.rs gates both assignments to result.primary_side_effects_data inside finalize_result on is_primary (true only for the first path yielded by path_pair.iter()), and test/bundler/esbuild/dce.test.ts adds three itBundled fixtures covering the array, glob, and inverse cases with a "name"-bearing package.json (which is what makes the secondary iteration reach the write site).

The Rust change is a re-indent of the existing match under an if is_primary plus && is_primary on the second block; it also merges the Glob(_) and Mixed(_) arms of the first match, whose bodies were identical. No other behavior in the loop (symlink resolution, tsconfig merge, module_type, result.package_json update) is touched.

Security risks

None. This is bundler tree-shaking metadata; the change makes DCE strictly less aggressive for the affected shape (keeps a bare import that was previously dropped). No untrusted-input parsing, no allocation, no FFI/GC.

Level of scrutiny

Low-to-moderate. finalize_result is on the resolver hot path, but the diff is a boolean guard around existing code with no new control flow. I confirmed PathPairIter yields primary at index 0 (result.rs:44-77), so core::mem::replace on a true-initialized flag is the correct idiom. Full dce.test.ts and packagejson.test.ts reportedly pass; the earlier CI build's only failure was an unrelated setInterval ASAN timeout.

Other factors

My previous review's only finding was a comment-style nit, which the author addressed in 6d2172f (resolver comment removed, test comment trimmed to issue URL + the load-bearing "name" invariant). All inline threads are resolved. The overlap with open PR #33883 is a maintainer coordination question, not a correctness concern — this PR is the narrow slice and stands on its own.

[robobun]

Closing in favor of #33883, which includes the same is_primary guard plus the broader enclosing_package_json fix. Rebasing that one onto current main.