Repository navigation
Conversation
The resolver branch that points a Node builtin at its browser polyfill returned a result with the default side-effect data. The bundler then kept the whole polyfill for an import statement that nothing uses. Each polyfill record now has a side-effects value, and the branch copies it to the result. The side_effects field of the polyfill package.json records had no reader, so it is removed. The sys polyfill keeps its side effects. It is `export * from "util"`, and the linker does not initialize a util that another polyfill loads with require() for a binding that an importer takes through a file with no side effects.
…e declined When every onResolve callback returns nothing, the bundler resolves the import itself in run_resolver. That path dropped the side-effect data of the resolver in two places. enqueue_parse_task stored only the loader's data for a new module, and the return for an external import did not set IS_EXTERNAL_WITHOUT_SIDE_EFFECTS. A package with "sideEffects": false, a Node builtin polyfill, or an external Node builtin then stayed in the bundle for an import that nothing uses. Both places now keep the data the resolver found, as the path with no plugin does. Where the resolver found none, a new module still gets the loader's data.
|
Updated 9:41 PM PT - Oct 25th, 2026
✅ @robobun, your commit d3c8fe8f5a7989194916954a32fcf3926771a07e passed in 🧪 To try this PR locally: bunx bun-pr 46783That installs a local version of the PR into your bun-46783 --bun |
|
Status: draft. It waits for a maintainer's decision and for #46786. A self-review rejected the larger variant that I built after the first push (a new How I reproduced it, on main (6292c65): echo 'import { createHash } from "node:crypto"; console.log("no use of createHash");' > unused.js
echo 'console.log("no import");' > none.js
bun build unused.js --target=browser --outfile=out-unused.js # 920,959 bytes
bun build none.js --target=browser --outfile=out-none.js # 37 bytesWith this PR |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Fallback side-effects metadata src/resolver/node_fallbacks.rs, src/resolver/resolver.rs |
Fallback specs now supply side-effects values, and the resolver copies the selected fallback module’s value into the resolution result. The sys fallback is marked as having side effects. |
Bundler side-effects handling src/bundler/bundle_v2.rs |
The bundler uses resolver metadata to set the external-without-side-effects flag. It uses loader metadata for input files when resolver metadata indicates side effects. |
Tree-shaking behavior tests test/bundler/bundler_browser.test.ts, test/bundler/bundler_plugin.test.ts |
Tests cover omitted unused imports and retained used imports, including code splitting, dynamic imports, require(), and plugin resolution callbacks. |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
2f772
-
No concrete issue remains that needs resolution before merging; normal validation still applies.
Pre-merge checks: 
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly identifies the main change: marking Node builtin polyfills as free of side effects to enable removal of unused imports. Description check The description follows the repository template with both required sections, "What does this PR do?" and "How did you verify your code works?". It states the problem, the fix, and the background, and … Full details: Description check
-
Explanation
The description follows the repository template with both required sections, "What does this PR do?" and "How did you verify your code works?". It states the problem, the fix, and the background, and it names the behavior changes and limits. It also gives concrete measurements and names the tests that cover the change. The author marks the PR as a draft. The author also asks a maintainer one open question about load order, and the notes say that question stays open.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the two refactor-shaped edits in src/resolver/node_fallbacks.rs: dropping side_effects: SideEffects::False from the synthetic PackageJSON is behavior-preserving (the only reader of package_json.side_effects is the real-directory path in resolver.rs around line 1623, which the fallback branch returns before reaching), and the zip(&SPECS) pairing is sound because MODULES is built from SPECS.iter() in the same order. The no-plugin external branch at bundle_v2.rs:7190 already stores the same side-effects bit, so the new run_resolver store matches it rather than introducing a divergent third site.
Extended reasoning...
The change adds a per-polyfill side-effects value in src/resolver/node_fallbacks.rs, copies it into the resolver result on the node-fallback branch, and makes the plugin-declined run_resolver/enqueue_parse_task path in src/bundler/bundle_v2.rs honor resolver-declared side-effects data for external records and new input files. It touches no security-sensitive surface. Three confirmed findings (splitting chunk-order change for used polyfills, order-dependent side-effects on mixed answered/declined imports, and a pre-existing ExternalRewritePath miss) are posted inline, so a human look is already signaled; this body only records what else was examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
|
|
||
| macro_rules! fallback_spec { | ||
| ($name:literal, $code_path:literal) => { | ||
| fallback_spec!($name, $code_path, SideEffects::NoSideEffectsPureData) |
There was a problem hiding this comment.
🔴 Browser builds with --splitting that use a Node polyfill can now run the polyfill after a later-imported file with side effects, so source evaluation order is no longer honored. Every polyfill except sys is declared NoSideEffectsPureData at src/resolver/node_fallbacks.rs:88, and the linker's chunk_of_file in findAllImportedPartsInJSOrder.rs:30 treats such a chunk as running nothing and moves its import behind chunks that do. Polyfills like url, util, buffer and timers read globals at load, so a setup file imported after them can now replace or freeze what they capture. Fix: keep the tree-shaking value for dropping unused imports but have chunk ordering treat polyfill files as running code at load, so a used polyfill keeps its source position.
Why this was flagged
A browser build with splitting: true and several entry points where entry.js has import { EventEmitter } from "node:events"; import "./setup.js"; and setup.js freezes a prototype or replaces a global such as globalThis.URL. src/resolver/node_fallbacks.rs:88 marks the events, url, util, buffer, timers and 18 other polyfills NoSideEffectsPureData. In src/bundler/linker_context/computeCrossChunkDependencies.rs:360-367 a chunk whose files all satisfy loading_file_has_no_side_effects is marked inert, and its import is placed after chunks that run code. The polyfill chunk is therefore evaluated after setup.js even though the source imported it first. On the base branch the polyfill package.json carried no usable side-effect data, so the chunk kept source order for these eight polyfills and the polyfill captured the original global or ran before the freeze. The new test test/bundler/bundler_browser.test.ts polyfill-first.js case asserts "ReplacedURL", locking in the reordered order.
Verification: src/resolver/node_fallbacks.rs:88 defaults every polyfill to SideEffects::NoSideEffectsPureData. The cross-chunk import ORDER is decided in src/bundler/linker_context/findAllImportedPartsInJSOrder.rs:29-33, which populates chunk_of_file only for files where !loading_file_has_no_side_effects(source_index). So the polyfill chunk's import is printed after every chunk that was reached through a side-effectful file.
There was a problem hiding this comment.
This is still open, and it is the question in the PR description. The branch is back to the resolver-only change, so a used polyfill orders like a "sideEffects": false package under --splitting. browser/UsedNodePolyfillLoadsAfterSideEffectsWithSplitting pins that.
I built the variant that you describe: a separate value that chunk ordering reads as code that runs at load. A self-review rejected it. It needs 128 added source lines of linker state, and it keeps the polyfill in a build that uses a polyfill which the unused one requires. The notes in the description have its numbers. A maintainer decides which of the two to take.
…onResolve declined" This reverts commit 2f772af.
… polyfill, and that no polyfill writes a global
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Keeps its side effects: it is `export * from "util"`, and the linker does not | ||
| // initialize a `require()`d `util` for a binding imported through a file without them. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Agreed. The exception for sys is a workaround for a linker gap on main: a name read through export * does not initialize a lazily loaded target. #46786 fixes that gap. This PR is a draft until it merges. The exception and its comment go then.
robobun
changed the title
Related to #14164
Draft: waits for a maintainer's answer (Downsides) and #46786.
Problem
bun build --target=browserbundles a whole Node builtin polyfill for an unused import: 921,016 bytes for a module whose server-only export callscreateHash, 107 without the import.process.browserdefine leaves the polyfill of the server branch that it removes (63,107 bytes against 118).src/resolver/resolver.rs:1949) returns no side-effect data. kit: implement server components dev server #14025 set a field for it that no code reads.Fix
"sideEffects": falsepackage. The linker then drops an unused import statement.syskeeps its side effects until bundler: keep theexport *statement of a lazily loaded module when a name is read through it #46786 merges: main loses theutilinitializer behind itsexport *.test/bundler/bundler_browser.test.ts(12 new tests, 9 fail on main).SideEffectsvalue, 128 added source lines against 14).Background
--target=browserthe resolver replacesnode:cryptoand 22 other builtins with polyfills inside Bun. They read globals at load and write none.Downsides
"sideEffects": falsepackage does? Notes has the three cases. Nobody reported the size cost, so "close" is a valid answer.export *statement of a lazily loaded module when a name is read through it #46786 merges,export *in a"sideEffects": falsepackage can leave a used polyfill uninitialized (TypeError: Buffer.from is not a function).Notes
Numbers are from builds of main (6292c65) with and without the change.
The question
export *statement of a lazily loaded module when a name is read through it #46786 merges, this PR becomes one resolver line (SideEffects::NoSideEffectsPackageJsonon the polyfill hit) plus tests, and the exception forsysgoes. I built that combination locally. Thesystest of this PR passes without the exception, and theexport *cases below print what main prints.side_effectson each polyfill record.The three cases where a used polyfill loses its load position
--splitting, the chunk of the polyfill can load after a chunk that a later import statement loads.import { URL } from "node:url"; import "./replace-url.js"; console.log(URL.name)in a build with four entry points printsReplacedURL. Main printsURL. A usednode:eventsbefore a file that callsObject.freeze(Object.prototype)throwsTypeError: Attempted to assign to readonly property.import "node:string_decoder"; import "./replace-blob.js"; import { Blob } from "node:buffer"printsReplacedBlob. Main printsBlob. Main printsReplacedBlobtoo when the first line is deleted.--splitting, a dropped polyfill no longer puts two chunks in order, and main has no order between sibling chunks. In one fixture a file then runs before a file that the source imports first (TypeError: undefined is not an object). Main fails the same way when the unused import is deleted.The tests
browser/UsedNodePolyfillLoadsAfterSideEffectsWithSplittingandbrowser/UnusedNodePolyfillImportDoesNotLoadPolyfillThatItRequirespin cases 1 and 2.A
"sideEffects": falsepackage has case 1 on main today. Withconsole.log("pure")in such a package andimport { value } from "pure"; import "./side.js"in one of three entry points,--splittingprintssidebeforepure.Unused imports
import { createHash } from "node:crypto"; console.log("x")in a.jsfile: 920,959 to 50 bytes.node:cryptonext to a usednode:buffer: 920,985 to 53,500 bytes.With #46786 (a local build of both changes)
export * from "node:buffer"in a"sideEffects": falsepackage, next to an unusednode:string_decoder: this PR alone throwsTypeError: Buffer.from is not a function. With bundler: keep theexport *statement of a lazily loaded module when a name is read through it #46786 it prints what main prints, in 53,639 bytes against 63,152.node:assertnext toinspectread throughexport * from "node:util"in such a package: this PR alone throwsTypeError: inspect is not a function. With bundler: keep theexport *statement of a lazily loaded module when a name is read through it #46786 it prints{ a: 1 }, in 10,966 bytes against 149,561.node:sysnext tonode:assert, with no exception forsys: prints what main prints.The larger variant (built and measured, not pushed)
SideEffects::RemovableIfUnusedfor the polyfills. Chunk assignment and chunk ordering read it as "has side effects". Tree shaking defers an import statement of such a file until the file, or a file that it reaches through import records, is live for another reason.--splittingchange module order when a polyfill is dropped.node:cryptonext to a usednode:bufferstays at 920,985 bytes.Limits that stay
node:fsand the other builtins with no polyfill: an unused import leavesvar {readFileSync} = (() => ({}));.require()andimport()keep the polyfill.onResolvecallback returns nothing, the polyfill stays (13,653 bytes against 35 for an unusednode:events). bundler: resume the shared resolve pass for imports that went to onResolve plugins #45408 owns that path.--ignore-dce-annotationskeeps every polyfill."browser"map ("crypto": false) already removes a polyfill by hand.