Conversation
|
Updated 4:05 AM PT - Aug 1st, 2026
❌ @robobun, your commit ba0c09f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33883That installs a local version of the PR into your bun-33883 --bun |
WalkthroughThe resolver now treats the nearest ChangesPackage.json resolution
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the fix looks correct and well-tested, but since enclosing_package_json feeds several resolver consumers beyond type/sideEffects (e.g. the version-specifier gate at resolver.rs:3125 and the auto-install path), a maintainer should confirm the broadened population is intended across all of them.
Extended reasoning...
Overview
This PR removes the !pkg.name.is_empty() || self.care_about_bin_folder guard at both dir_info_uncached sites in src/resolver/resolver.rs so that enclosing_package_json is always recorded for the nearest package.json, regardless of whether it has a "name" field. It also updates the doc comment on DirInfo::enclosing_package_json and adds three itBundled tests covering the root-dir path, the parent-propagation path, and a with-name control.
The stated motivation is that finalize_result reads "type" and sideEffects off enclosing_package_json, and Node's module-format rule considers only the nearest package.json's "type" — "name" is irrelevant. The PR notes this re-applies d862105 from the Zig resolver that was dropped in the Rust port.
Security risks
None identified. The change only affects which already-parsed PackageJSON object is recorded on a DirInfo; there is no new I/O, no new untrusted-input parsing, and no auth/crypto surface.
Level of scrutiny
Medium-high. The diff itself is tiny and the logic is clearly correct for the headline case ("type" classification). However, enclosing_package_json is read at ~10 sites across resolver.rs, jsc_hooks.rs, run_command.rs, and multi_run.rs. Most are neutral or improved (the bun run consumers already ran with care_about_bin_folder=true, and the jsc_hooks sites want exactly this behavior — #33807 worked around its absence). But at resolver.rs:3125, enclosing_package_json.is_some() gates the "VersionSpecifierNotAllowedHere" error on the global-cache auto-install path; that branch will now fire for nameless-package.json directories where it previously fell through. That's plausibly the right behavior (a nameless package.json is still a package boundary), and is presumably what the original Zig fix did too, but it's a behavior change not covered by the new tests.
Other factors
- No CODEOWNERS on
src/resolver/. - Bug-hunting system found no issues.
- Tests are well-constructed: they exercise both write sites (own-dir and parent-propagation) plus a control, and assert observable bundle output rather than internals.
- CI build was still in progress at review time.
Given the resolver's blast radius and the secondary consumer at line 3125, I'd like a maintainer to sign off rather than auto-approve.
|
Consumer audit for the broadened
The diff is the same two-site guard removal as d862105; every consumer either already saw the broadened population (via |
There was a problem hiding this comment.
I didn't find issues with the change, but this broadens enclosing_package_json population across all resolver consumers and reworks the sideEffects loop in finalize_result — the same guard removal was previously attempted and closed (#22211), so a maintainer familiar with the resolver should confirm the consumer audit and the iteration-2 result.package_json interaction.
Extended reasoning...
Overview
This PR touches the module resolver (src/resolver/resolver.rs, src/resolver/dir_info.rs) and adds four bundler tests. Two native changes:
- Removes the
!name.is_empty() || care_about_bin_folderguard at bothdir_info_uncachedsites soenclosing_package_jsonis always the nearest package.json, matching Node's module-format rule. - Adds a
side_effects_donelatch infinalize_resultsoprimary_side_effects_datais computed only againstpath_pair.primaryand not overwritten by the secondary (main/module dual-package) path. This also gates theresult.package_json = Noneclear and the first sideEffects block behind!side_effects_done, so they no longer run on iteration 2.
Security risks
None identified. No untrusted-input parsing, auth, crypto, or filesystem-write paths are touched.
Level of scrutiny
High. enclosing_package_json is read by bun run, bun build, the runtime module loader (jsc_hooks.rs), and the auto-install gate — the blast radius covers essentially every module resolution. The PR description notes that #22211 attempted the identical guard removal and was closed because it broke the PackageJsonSideEffectsArrayKeep* tests; this PR pairs the removal with the finalize_result fix that addresses that. The consumer audit posted on the PR looks thorough and the reasoning is sound, but this is exactly the kind of resolver-semantics change where a maintainer with context on the Zig→Rust port and the #229/#22211 history should sign off.
Other factors
- The
&& !side_effects_doneaddition to theif let Some(existing) = ...block also skips theexisting.name.is_empty() → result.package_json = Noneclear on iteration 2. In the common case (primary and secondary in the same package dir) this is a no-op, but it is a second-order behavior change beyond the sideEffects computation itself and deserves a human eye. - Test coverage is good: three new
packagejson/TypeCommonJS*cases (fail-before/pass-after + control) and one newdce/...NamedPackagecase that reproduces the latent bug onmain. The description reports the fulldce,packagejson,default,bundler_cjs*,bundler_browser, andtest/js/bun/resolve/suites pass. - No CODEOWNERS entry matches these paths.
|
On the iteration-2 After iteration 1, Case-by-case against
So the only case whose outcome changes is the one the PR is fixing; the other cases end up with the same |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/resolver/resolver.rs`:
- Around line 1625-1634: In the path-iteration logic, mark the primary/secondary
side-effect classification as consumed before attempting read_dir_info. Update
the state immediately after obtaining each path name and before the
read_dir_info call, then remove the later assignment so early continue paths
cannot leave side_effects_done false; apply the same adjustment to the
corresponding logic near the secondary occurrence.
🪄 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: 8550608d-9486-498f-b364-ecb889fec49d
📒 Files selected for processing (4)
src/resolver/dir_info.rssrc/resolver/resolver.rstest/bundler/esbuild/dce.test.tstest/bundler/esbuild/packagejson.test.ts
There was a problem hiding this comment.
I didn't find any bugs, but this changes core resolver semantics (enclosing_package_json population + the finalize_result sideEffects loop) in a way a prior attempt (#22211) got wrong — worth a human look at the iteration-2 / care_about_bin_folder interaction and the broadened consumer set.
Extended reasoning...
Overview
Two coupled resolver changes: (1) dir_info_uncached now records enclosing_package_json unconditionally at both sites, dropping the !name.is_empty() || care_about_bin_folder guard; (2) finalize_result now latches is_primary before the read_dir_info early-continue and gates both sideEffects blocks on it, so primary_side_effects_data is computed only from path_pair.primary. Doc comment updated in dir_info.rs. Four new bundler tests (three packagejson/TypeCommonJS*, one dce/PackageJsonSideEffectsArrayKeepModuleNamedPackage).
Security risks
None identified. This is module-resolution metadata plumbing; no auth, crypto, permissions, or untrusted-input parsing surface is touched.
Level of scrutiny
High. Module resolution is a critical, load-bearing path — enclosing_package_json feeds "type" classification (CJS vs ESM) and sideEffects/DCE for every bundled file, and is read by run_command.rs, jsc_hooks.rs, and the auto-install gate. The fix is a semantic broadening (nameless package.json now counts as a package boundary everywhere), and a previous attempt at exactly this guard removal (#22211) was closed after regressing sideEffects — the second half of this PR is the fix for that latent regression. That coupling is the kind of thing a maintainer familiar with the resolver should sanity-check.
Other factors
- The PR thread contains a thorough consumer audit and an iteration-2
result.package_jsoncase analysis from robobun; both look sound to me, but they hinge on invariants about whenresult.package_jsonis populated pre-loop and whether the.or_else(dir.enclosing_package_json)fallback is now always non-None — worth a maintainer confirming. - The
if existing.name.is_empty() || self.care_about_bin_folder { result.package_json = None; }at resolver.rs:1674 is now only reachable on iteration 1 (gated bycompute_side_effects); thecare_about_bin_folderarm of that clear now behaves differently on iteration 2 vs before. The robobun analysis argues this is a no-op in practice because iteration 1's fallback already populatedresult.package_json, but I'd want a human to confirm that reasoning. - CodeRabbit's early-continue ordering issue was addressed in 312fae0 and the thread is resolved.
- Test coverage is targeted and includes a control case; the author reports the broader bundler/resolve suites pass locally.
|
CI status: the bundler and runtime test lanes touched by this diff are green;
None touch the resolver or module-format classification. Ready for review. |
|
Added runtime coverage in Also merged |
There was a problem hiding this comment.
Thanks for applying the combined-object assertion — that resolves the only nit from the last pass. No bugs found this run. I'm deferring rather than auto-approving because this changes resolver semantics on a hot path (broadens enclosing_package_json population + reworks the finalize_result per-iteration state that a prior attempt at the same guard removal tripped over), and a maintainer familiar with the resolver should sign off.
What was reviewed:
- The two
dir_info_uncachedguard removals and their downstream consumers (run_command,jsc_hooks, the auto-install gate) — the audit posted in-thread checks out. - The
finalize_resultiteration-2 behavior: withcompute_side_effectsgating theexistingblock, the nameless-clear at L1661 no longer fires on the secondary;result.package_jsonstays as iteration 1 left it, which matches the case-by-case analysis posted earlier. - The
is_primarycapture-before-read_dir_infofix for the early-continuepath. - New tests: bundler (
packagejson/TypeCommonJS*,dce/...NamedPackage) and runtime (run-cjs.test.ts) — each has a control case and the failing cases use observable behavior (namespace shape / top-levelreturn).
Extended reasoning...
Overview
Two changes to src/resolver/:
dir_info_uncachednow recordsenclosing_package_jsonunconditionally at both sites, dropping the!name.is_empty() || care_about_bin_folderguard. This aligns with Node's rule that the nearest ancestorpackage.json's"type"governs.jsmodule format regardless of whether it has a"name".finalize_resultgains anis_primaryflag (captured and cleared before the fallibleread_dir_infocall) soprimary_side_effects_datais computed only againstpath_pair.primary, not overwritten by the secondary. This latent bug was previously masked by the name guard for the nameless-package.json fixtures used in the existing DCE tests; the newdce/PackageJsonSideEffectsArrayKeepModuleNamedPackagetest shows it was already broken onmainfor named packages.
Test coverage spans the bundler path (three packagejson/TypeCommonJS* cases + one DCE case) and the runtime path (four run-cjs.test.ts cases with controls). The doc comment on DirInfo::enclosing_package_json is updated to match.
Security risks
None identified. This is module-resolution metadata bookkeeping; no untrusted-input parsing, no filesystem writes, no auth/crypto surface.
Level of scrutiny
High. The resolver is a critical shared path — every module load and every bundle goes through it — and enclosing_package_json has multiple consumers (run_command, jsc_hooks, the bundler finalize_result, the auto-install VersionSpecifierNotAllowedHere gate). A previous attempt at the identical guard removal (#22211) was closed, and this PR itself discovered a latent overwrite bug that the guard had been masking. The finalize_result loop still contains the existing.name.is_empty() || care_about_bin_folder clear-then-.or_else-restore dance (L1661–1668), whose interaction with the new compute_side_effects gate on iteration 2 is subtle enough that the author posted a dedicated case-by-case analysis. That reasoning holds up on inspection, but this is exactly the kind of state-machine change a resolver maintainer should confirm.
Other factors
- All prior review threads (CodeRabbit's early-
continueconcern, my combined-assertion nit) are resolved and applied. - The consumer audit in-thread is thorough and matches what I see in the code.
- CI bundler lanes are reported green; remaining red is documented as unrelated infra flake.
- Tests follow harness conventions (
tempDir,bunEnv, concurrent pipe drain, combined-object assertion) and include controls that pass on bothmainand the branch.
|
This also fixes #8993 ( Those packages have |
bun build was ignoring {"type":"commonjs"} in a package.json that had no
"name" field, so a .js file that the unbundled program (and Node) treat
as CommonJS was bundled as ESM: a namespace import came back empty and
its default member was undefined. Adding an unrelated "name" field to
the same package.json made the bundle correct.
dir_info_uncached only stored a directory's enclosing_package_json when
the package.json had a non-empty name (or care_about_bin_folder was set
for bun run). That guard was introduced for bun run script discovery,
but enclosing_package_json is also what finalize_result reads the "type"
and sideEffects fields from, so a nameless package.json lost its type in
the bundler. Node's module-format rule looks only at the nearest
package.json's "type"; "name" is irrelevant.
Always record the nearest package.json as enclosing_package_json. This
re-applies d862105, which made the same change in resolver.zig but
was not carried into the Rust port.
The path_pair loop in finalize_result evaluates the sideEffects
classification for both the primary ("module") and secondary ("main")
paths of a main/module resolution, with the secondary's result
overwriting the primary's. primary_side_effects_data is paired with
path_pair.primary everywhere it is consumed (ParseTask::init,
enqueue_entry_item, resolve_import_records); the secondary path gets
its own resolve result if it is ever reached via require().
The previous commit exposed this by keeping result.package_json
populated on the second iteration for a nameless package.json, where
it was previously cleared and the second-iteration sideEffects write
skipped. A named package.json already took the same broken path (new
dce/PackageJsonSideEffectsArrayKeepModuleNamedPackage test fails on
main).
If read_dir_info failed for the primary path, the continue skipped the latch update and the secondary iteration would still compute primary_side_effects_data. Capture the first-iteration flag before the early exit so the secondary is never treated as primary.
…ubdir
Exercises the runtime face of the enclosing_package_json name gate: a .js
entry in a subdirectory of a nameless {"type":"commonjs"} scope was being
classified by the next named ancestor instead. The top-level-return body
makes the misclassification observable as a SyntaxError from JSC.
Two fail-before cases (with and without a named outer {"type":"module"}
ancestor) and two controls (adjacent file; inner package.json has a name).
9f88494 to
3f9f1e0
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto current main (was 358 commits behind), trimmed the flagged comments, and folded in the bare-import sideEffects tests from #36652 covering the Locally: |
There was a problem hiding this comment.
No bugs found, but this is a resolver semantics change (enclosing_package_json population + finalize_result iteration ordering) where a prior attempt (#22211) was reverted, so it's worth a human sign-off.
What was reviewed:
- The two
name.is_empty()guard removals indir_info_uncachedagainst the consumer audit (run_command setscare_about_bin_folder, jsc_hooks reads via.or(), auto-install gate at 3125) — no unintended broadening found. - The
is_primarylatch infinalize_result: confirmed it clears beforeread_dir_infoso an earlycontinueon primary can't let secondary writeprimary_side_effects_data; iteration-2 skip of thename.is_empty()clear is a no-op because.or_elseat 1611 sees a populatedresult.package_jsonfrom iteration 1. - Tests: bundler + runtime coverage for both the nameless-
typeand the sideEffects-overwrite class; the four new dce tests exercise named packages (the shape #22211 broke).
Extended reasoning...
Overview
Two coupled resolver fixes plus tests:
dir_info_uncached(two sites): drop the!name.is_empty() || care_about_bin_folderguard soenclosing_package_jsonrecords the nearest package.json unconditionally. This is whatfinalize_resultreads"type"andsideEffectsfrom; Node's module-format rule ignores"name".finalize_result: latchis_primarybeforeread_dir_infoand gate both sideEffects blocks on it, soprimary_side_effects_datais computed only forpath_pair.primary. The old code overwrote it with the secondary ("main") path's classification, which the name-guard had been accidentally masking for nameless packages.
Tests: 3 bundler cases in packagejson.test.ts (nameless type:commonjs at root/subdir + named control), 4 dce cases covering the named-package sideEffects overwrite and the tfjs bare-import shape from #8993, and 4 runtime cases in run-cjs.test.ts using top-level return as the CJS-vs-ESM observable.
Security risks
None. Module-format classification and tree-shaking metadata; no untrusted input parsing, auth, or filesystem-write surface changes.
Level of scrutiny
High. The resolver is on every import path for both bun build and bun run, and enclosing_package_json fans out to ~10 consumers. A semantically similar earlier attempt (#22211) was closed — this PR explains why (the sideEffects overwrite it exposed) and fixes that too, but that history is exactly why a maintainer should confirm the iteration-2 reasoning: on secondary the existing.name.is_empty() || care_about_bin_folder clear at line 1606 no longer runs, and correctness relies on the .or_else(dir.enclosing_package_json) at 1611 being a no-op because iteration 1 always leaves result.package_json populated (which in turn depends on fix #1). The chain holds, but it's the kind of coupled invariant a resolver owner should eyeball.
Other factors
- Consumer audit already posted in-thread and re-checked here:
bun runsetscare_about_bin_folderbefore every read, so no behavior change there;jsc_hooks.rsgains Node-correcttypeinheritance (which #33807 currently walks parents to work around); the auto-installVersionSpecifierNotAllowedHeregate now fires for nameless-package.json boundaries too, matching the reference Zig resolver. - CodeRabbit's early-
continueconcern was addressed (latch moved beforeread_dir_info); my earlier nit on combined-object subprocess assertions was applied. - CI: bundler/resolver lanes green across five builds; the one remaining red (
test-net-connect-memleak.json Alpine x64) is a known flake tracked separately and unrelated to this diff. - The PR description notes #33807 works around the same field at the runtime layer; a maintainer may want to decide whether that workaround should be unwound in a follow-up.
…ed or not (#41232) ### Problem - Regression on main from #41150. No release has it. `bun build` reads `"type"` from the wrong package.json, so it misses a `{ "type": "module" }` that has no `"name"`. Two common places: a project root, and a dual package's `dist/esm/package.json`. - A `.js` or `.ts` file there gets `exports.default` for the default import of a CommonJS module with `__esModule`. Node, esbuild and bun 1.4.0 give the whole `module.exports`. The bundle has `__toESM(require_tsdep())`, without `, 1`. - Cause: `finalize_result` (`src/resolver/resolver.rs:1669`) read `"type"` from the package root, or else from `enclosing_package_json`. `dir_info_uncached` (`src/resolver/resolver.rs:6357`) sets that field only for a named package.json (#229). ### Fix - `DirInfo` gets `package_json_for_module_type`: the nearest package.json in the directory or above it, named or not. `finalize_result` reads `"type"` from it for the primary path. The extension still wins. - Correct because esbuild uses this rule, and Node ignores `"name"` too. - Only the bundler reads `Result.module_type`. `enclosing_package_json` does not change. The four runtime lookups in `src/runtime/jsc_hooks.rs` are out of scope. - Verified: `test/bundler/bundler_cjs.test.ts`, 10 new cases, 9 fail on main. Self-reviewed: 3 concerns raised, 3 addressed. Other suites in Notes. ### Background - `__toESM(mod, isNodeMode)` builds the ESM view of a CommonJS module. With `, 1` (Node mode), `default` is the whole `module.exports`. Without it, `default` is `mod.default` when `__esModule` is set. - `DirInfo` is the resolver's cached record for one directory. Its "enclosing" fields come from the parent. - Open PRs in this area: #33883, #33807, #33890, #40940. This PR supersedes none. Notes cover #40940. <details><summary>Notes</summary> Found by comparing `bun build` on main with bun 1.4.0, Node 26 and esbuild 0.25. No issue is open for it. Repro for the dual-package face: ```sh D=$(mktemp -d); cd $D; mkdir -p node_modules/pkg/dist/esm node_modules/tsdep echo '{"name":"tsdep","version":"1.0.0","main":"index.js"}' > node_modules/tsdep/package.json echo 'Object.defineProperty(exports,"__esModule",{value:true}); exports.default=function styled(){}; exports.css="css";' > node_modules/tsdep/index.js echo '{"name":"pkg","version":"1.0.0","main":"./dist/esm/index.js"}' > node_modules/pkg/package.json echo '{"type":"module"}' > node_modules/pkg/dist/esm/package.json echo 'import styled from "tsdep"; export const seen = typeof styled + "/" + typeof styled.default;' > node_modules/pkg/dist/esm/index.js echo 'import { seen } from "pkg"; console.log(seen);' > app.mjs node app.mjs # object/function bun build ./app.mjs --target=node --outfile=out.mjs && node out.mjs # main: function/undefined, this PR: object/function ``` For the project-root face, put `{ "type": "module" }` (no `"name"`) in the project's package.json and bundle a `.js` file that imports `tsdep`. Faces of the bug on main. Each has a test: - A project package.json with `"type"` and no `"name"` (case 58). - The nested marker reached through `"main"`, `"module"` or a relative path (cases 53, 55, 56). Through an exports map it worked, because `handle_esm_resolution` reads the file's own directory. - A nested package.json with a `"name"`, reached through `"main"` (case 54). `result.package_json` was the package root, so the nested file was not read at all. - A file in a subdirectory of the marker (case 57). Why a new field instead of widening `enclosing_package_json`: that field also names the package for `sideEffects`, the auto-install version gate and `bun run` script discovery. #33883 widens it for every consumer and had to rework the `sideEffects` loop in `finalize_result` to keep the DCE tests passing. Four cases pin the lookup rule. Each result matches esbuild 0.25.1: - Case 59: a nameless `{ "type": "commonjs" }` below a `"type": "module"` package wins, because it is the nearest. - Case 60: a nearest package.json without `"type"` is the scope. The lookup does not continue to a typed package root, so the importer is not ESM by type. Main read the root's `"type"` here. Node prints `object/function` for this shape, but only because it detects ESM syntax in a file with no `"type"`. #41150 chose the esbuild rule for such files. - Case 61: the lookup does not stop at a `node_modules` directory. A package without a package.json of its own takes the `"type"` above it. Node prints the same result. - Case 62: only the primary path decides. With the default target, `"module"` is the primary path and `"main"` is the fallback for `require()`. The fallback's package.json does not count. esbuild has the same check. The case fails when the check is removed. Overlap with #40940: it adds a field with the same name, but its lookup stops at `node_modules`, and the runtime reads it too. If #40940 lands after this PR, it must choose one rule for the field. Case 61 pins the crossing for the bundler. Node's stop can still apply at the runtime read sites. #40940 also calls the lookup for the fallback path, which case 62 rejects. Out of scope: the runtime's four lookups (`src/runtime/jsc_hooks.rs` lines 1474, 2972, 3220 and 4071) keep `package_json().or(enclosing_package_json)`. So `bun run` still skips a nameless package.json above the file's own directory. That gap predates #41150. For this import, `bun run` 1.4.1 gives `exports.default` for every importer, even `.mjs`. Self-review, the three concerns and what changed: - Document the `node_modules` rule on the field. Done in `src/resolver/dir_info.rs`. - Add a default-target case with both `"main"` and `"module"`. That is case 62. - Say in this body that no release has the bug, lead with the project-root face, and name the runtime lookups that stay. Suites run with the fix on a debug ASAN build, after a rebase on main: `bundler_cjs` (62), `esbuild/packagejson`, `esbuild/dce`, `esbuild/default`, `bundler_cjs2esm`, `bundler_npm`, `bundler_edgecase`, `bundler_regressions`, `bundler_splitting`, `bundler_barrel`, `cli/run/run-cjs`, `test/js/bun/resolve`. All pass except the second case of `test/js/bun/resolve/load-same-js-file-a-lot.test.ts`. It times out at 5 s on this build with and without this change (back-to-back runs on the same machine). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 9 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_cjs.test.ts bun test v1.4.1 (a6c4cc2) test/bundler/bundler_cjs.test.ts: (pass) bundler > cjs/__toESM_import_syntax_with_esModule [945.38ms] (pass) bundler > cjs/__toESM_import_syntax_without_esModule [445.45ms] (pass) bundler > cjs/__toESM_import_syntax_function [371.94ms] (pass) bundler > cjs/__toESM_import_syntax_primitive [387.72ms] (pass) bundler > cjs/__toESM_import_syntax_named_and_default [361.78ms] (pass) bundler > cjs/__toESM_import_syntax_namespace [367.76ms] (pass) bundler > cjs/__toESM_target_node [455.64ms] (pass) bundler > cjs/__toESM_target_browser [413.38ms] (pass) bundler > cjs/__toESM_target_bun [455.22ms] (pass) bundler > cjs/__toESM_format_esm [462.40ms] (pass) bundler > cjs/__toESM_format_cjs_with_import [377.93ms] (pass) bundler > cjs/__toESM_mjs_reexport [431.60ms] (pass) bundler > cjs/__toESM_mjs_reexport_with_esModule [432.63ms] (pass) bundler > cjs/__toESM_deep_reexport_chain [368.70ms] (pass) bundler > cjs/__toESM_reexport_with_rename [443.84ms] (pass) bundler > cjs/__toESM_default_prop ... (truncated) release without fix: 20 FAILED bun test v1.4.1-canary.1 (a6c4cc2) test/bundler/bundler_cjs.test.ts: runtime failed file: /tmp/bun-build-tests/bun-4t1lr0/cjs/__toESM_import_syntax_with_esModule/out.js stdout output: {"__esModule":true,"default":{"value":"default export"},"named":"named export"} --- expected stdout: {"value":"default export"} --- 1913 | console.log(`---`); 1914 | console.log(`expected ${name}:`); 1915 | console.log(expected); 1916 | console.log(`---`); 1917 | } 1918 | expect(result).toBe(expected); ^ error: expect(received).toBe(expected) Expected: "{"value":"default export"}" Received: "{"__esModule":true,"default":{"value":"default export"},"named":"named export"}" at <anonymous> (/workspace/bun/test/bundler/expectBundled.ts:1918:28) (fail) bundler > cjs/__toESM_import_syntax_with_esModule [29.17ms] (pass) bundler > cjs/__toESM_import_syntax_without_esModule [11.71ms] (pass) bundler > cjs/__toESM_import_syntax_function [9.90ms] (pass) bundler > cjs/__toESM_import_syntax_primitive [9.43ms] (pass) bundler > cjs/__toESM_import_syntax_named_and_default [9.97ms] ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_cjs.test.ts bun test v1.4.1 (a6c4cc2) test/bundler/bundler_cjs.test.ts: (pass) bundler > cjs/__toESM_import_syntax_with_esModule [1024.06ms] (pass) bundler > cjs/__toESM_import_syntax_without_esModule [473.45ms] (pass) bundler > cjs/__toESM_import_syntax_function [489.71ms] (pass) bundler > cjs/__toESM_import_syntax_primitive [494.25ms] (pass) bundler > cjs/__toESM_import_syntax_named_and_default [388.89ms] (pass) bundler > cjs/__toESM_import_syntax_namespace [381.97ms] (pass) bundler > cjs/__toESM_target_node [389.92ms] (pass) bundler > cjs/__toESM_target_browser [453.17ms] (pass) bundler > cjs/__toESM_target_bun [481.14ms] (pass) bundler > cjs/__toESM_format_esm [415.53ms] (pass) bundler > cjs/__toESM_format_cjs_with_import [376.77ms] (pass) bundler > cjs/__toESM_mjs_reexport [451.67ms] (pass) bundler > cjs/__toESM_mjs_reexport_with_esModule [380.06ms] (pass) bundler > cjs/__toESM_deep_reexport_chain [453.02ms] (pass) bundler > cjs/__toESM_reexport_with_rename [430.08ms] (pass) bundler > cjs/__toESM_default_pro ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 822e3b2 features baseline 23 deps, 131 codegen, 1172 objects in 647ms ninja: Entering directory `/workspace/bun/build/release' [1/1244] install /workspace/bun bun install v1.4.1-canary.1 (a6c4cc2) Checked 25 installs across 62 packages (no changes) [10.00ms] [2/1244] gen bindgenv2 [3/1244] gen ErrorCode+*.h [4/1244] install /workspace/bun/packages/bun-error bun install v1.4.1-canary.1 (a6c4cc2) Checked 1 install across 2 packages (no changes) [3.00ms] [5/1244] fetch libjpeg-turbo [libjpeg-turbo] up to date [6/1217] gen ProcessBindingConstants.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp [7/1217] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [8/1217] fetch tinycc [tinycc] up to date [9/1216] install /workspace/bun/src/node-fallbacks bun install v1.4.1-canary.1 (a6c4cc2) Checked 111 installs across 104 packages (no changes) [15.00 ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/resolver/dir_info.rs | 9 ++ src/resolver/resolver.rs | 20 +++-- src/resolver/result.rs | 15 +--- test/bundler/bundler_cjs.test.ts | 182 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 207 insertions(+), 19 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/resolver/dir_info.rs 3 4 33 src/resolver/resolver.rs 8 5 34 src/resolver/result.rs 1 1 33 test/bundler/bundler_cjs.test.ts 3 11 33 ``` </details> <!-- robobun:evidence:end -->
This branch has not been deployed
1 participant
Fixes #8993
Repro
Unbundled, Node and Bun agree that
c.jsis CommonJS (namespace has adefault):Bundled,
bun buildclassifies the samec.jsas ESM:Adding an unrelated
"name":"p"to the same package.json makes the bundle correct:Cause
dir_info_uncachedonly stored a directory'senclosing_package_jsonwhen the package.json had a non-emptyname(orcare_about_bin_folderwas set, which is thebun runpath). That guard existed forbun runscript discovery (#229), butenclosing_package_jsonis also whatfinalize_resultreads the"type"andsideEffectsfields from, so a nameless package.json lost itstypein the bundler. Node's module-format rule looks only at the nearest package.json's"type";"name"is irrelevant. A nameless{"type":"commonjs"}is common in app-root and monorepo-internal package.json files.Removing that guard exposed a second, latent bug in
finalize_result: thepath_pairloop evaluates the sideEffects classification for both the primary ("module") and secondary ("main") paths of a main/module resolution, with the secondary's result overwriting the primary's.primary_side_effects_datais paired withpath_pair.primaryeverywhere it is consumed (ParseTask::init,enqueue_entry_item,resolve_import_records); the secondary path gets its own resolve result when it is reached viarequire(). The name guard had been accidentally masking this for nameless package.jsons by clearingresult.package_jsonbefore the second iteration, and everydce/PackageJsonSideEffectsArrayKeep*test happens to use a nameless one. A named package.json already took the same broken path onmain(newdce/PackageJsonSideEffectsArrayKeepModuleNamedPackagetest).(#22211 attempted the same guard removal and was closed; this is presumably why.)
Fix
enclosing_package_jsonat bothdir_info_uncachedsites.finalize_result, computeprimary_side_effects_dataon the firstpath_pairiteration only.Tests
test/bundler/esbuild/packagejson.test.ts:TypeCommonJSWithoutName/TypeCommonJSWithoutNameInSubdir:{"type":"commonjs"}with noname, at the root and inherited from a subdirectory; both fail before, pass after.TypeCommonJSWithName:{"name":"p","type":"commonjs"}control; passes before and after.test/bundler/esbuild/dce.test.ts:PackageJsonSideEffectsArrayKeepModuleNamedPackage: the existing...KeepModuleImplicitModulefixture with a"name"added; fails onmain, passes after.PackageJsonSideEffectsArrayKeep{Main,Module}*tests continue to pass.test/cli/run/run-cjs.test.ts(runtime):nameless package.json "type" governs module format: direct-run.jsin a subdirectory of a nameless{"type":"commonjs"}scope, with top-levelreturnas the observable (JSC rejects it when the file is misclassified as ESM). Two cases (with and without a named{"type":"module"}ancestor) fail before and pass after; adjacent-file and named-inner controls pass on both.Also adds bare-import variants (
dce/PackageJsonSideEffectsArrayModuleMainBareImport,GlobModuleMainBareImport,ArrayModuleMainBareImportRemove) covering the@tensorflow/tfjs-backend-cpushape from #8993 (folded in from #36652).dce.test.ts(74 pass),packagejson.test.ts(77 pass),default.test.ts(151 pass),bundler_cjs.test.ts,bundler_cjs2esm.test.ts,bundler_browser.test.tsandtest/js/bun/resolve/pass with no new failures.Related: #33807 works around the nameless-package.json skip for the runtime path by walking
DirInfoparents injsc_hooks.rs; this PR fixes the resolver field it was working around.no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/esbuild/dce.test.ts