Repository navigation
Conversation
Webpack (which defines the package.json sideEffects field) documents that array entries without a '/' are matched against the basename at any depth, equivalent to prefixing the pattern with '**/'. esbuild follows the same rule. Bun was joining such patterns directly onto the package directory, so '*.side.js' only matched files at the package root and a nested file the package declared as side-effectful was silently tree-shaken out of the bundle. When a sideEffects array entry contains no '/', store it as a glob with a '**/' prefix so it matches at any depth. Entries with an explicit path component (including a leading './') keep their existing root-relative behavior.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Resolver pattern classification and matching src/resolver/package_json.rs |
ExactEntries distinguishes package-relative paths from bare filenames. SideEffects::from_array builds exact, glob, or mixed representations. Bare glob patterns receive a **/ prefix, and path patterns are normalized. Matching checks the corresponding exact and glob entries. |
Bundler coverage for matching semantics test/bundler/bundler_barrel.test.ts, test/bundler/esbuild/dce.test.ts, test/bundler/esbuild/extra.test.ts, test/bundler/expectBundled.ts |
Tests cover bare filenames, globs, package-relative paths, package contexts, plugin-loaded files, and CSS behavior. The harness and test configurations remove the wildcard-side-effects skip option. |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
af8b8
-
Merge Risk: ⚪ Minimal · up to
af8b8The matching change is mergeable after normal checks; no concrete unresolved issue was identified.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the main change: matching slashless sideEffects entries in every directory and treating globs as recursive patterns. 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. Description check The description provides detailed problem, fix, scope, tradeoffs, and verification results. It does not use the template headings exactly, but it clearly covers both required topics: what the PR does … Full details: Description check
-
Explanation
The description provides detailed problem, fix, scope, tradeoffs, and verification results. It does not use the template headings exactly, but it clearly covers both required topics: what the PR does and how the code was verified.
-
- 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}
Now that slashless entries always route through the glob branch, a bare 'style.css' entry would be filtered out at parse time, producing an empty glob list. Bun keeps CSS imports regardless of the sideEffects field so this was not observable in the bundle output, but the parser should faithfully represent what package.json says rather than diverge between 'style.css' and './style.css'. Drop the skip from both glob branches.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the follow-up commits addressed all three prior findings (.css skip removed, comment shortened, dce: true added to the CSS+JS test).
What was reviewed:
- The
**/prefix is inserted only for slashless entries in both the mixed and glob-only branches; entries with/(including./) keep root-relative behavior — covered by theDotSlashStaysRootOnlyregression guard. join_string_bufskips the empty middle segment so theb""case is byte-identical to the pre-PR 2-element join.- Checked that removing the
.cssskip is safe: the exact-only branch never had it, so this unifies the three branches rather than changing net behavior for path-qualified CSS entries.
Extended reasoning...
Overview
This PR changes ~25 lines in src/resolver/package_json.rs inside the "sideEffects" array parser, and adds five itBundled tests (~130 lines) to test/bundler/esbuild/dce.test.ts. The core change: when a sideEffects array entry contains no /, it is now (a) classified as a glob in the first pass and (b) stored with a **/ segment inserted between the package directory and the pattern, so it matches at any depth. This matches webpack's documented semantics and esbuild's implementation. Previously such entries were joined directly onto the package root, so "*.side.js" only matched top-level files and nested side-effectful files were silently tree-shaken.
The follow-up commits also removed the pre-existing .css-extension skip from the glob and mixed branches (which the reclassification would otherwise have exposed as a regression for bare "style.css" entries), shortened the classifier comment per the comment-cop bot, and added dce: true / dceKeepMarkerCount: false to the CSS+JS test so the FAIL-marker grep runs on the outdir path.
Security risks
None. This is pure string classification over already-parsed package.json array items — no untrusted-length arithmetic, no allocation sizing from external counts, no filesystem or network I/O, no unsafe blocks touched. The worst-case failure mode (a mis-stored glob) is that a file is kept in the bundle when it could have been dropped, or vice versa, which is a correctness/output-size issue rather than a security one.
Level of scrutiny
Moderate — bundler DCE correctness matters (dropping a declared-side-effectful file is a silent runtime break), but the change is localized to one parsing function and follows a well-documented reference (webpack docs + esbuild). I traced the three code paths (mixed / glob-only / exact-only) against the new classifier: the exact-only branch is now only reachable when every entry contains a /, and the mixed branch's per-item routing (slashless || <metachars> → glob, else exact) is consistent with the first-pass classifier. I also verified that join_string_buf_t skips empty parts, so passing b"" as the middle segment for slash-containing entries produces byte-identical output to the previous 2-element join (no double-separator or normalization difference). The globstar zero-depth case (<pkg>/**/foo.js matching <pkg>/foo.js) is directly exercised by the root-level files in the new tests.
Other factors
All three findings from my prior review passes are resolved: the 🔴 .css-skip regression was fixed by removing the skip entirely (the exact-only branch never had it, so this is a unification); the 🟡 test-convention nit was addressed by adding dce: true with dceKeepMarkerCount: false (I confirmed in expectBundled.ts:1509 that false disables the outdir KEEP accounting while keeping the FAIL grep); and the 🟡 pre-existing absolute-path-glob-metachar exposure was acknowledged for a follow-up package-relative-matching redesign, which is the right scope boundary since that flaw already affected wildcard entries before this PR. The comment-cop bot's long-comment warnings were also addressed. Test coverage is thorough: glob-at-depth, bare-name-at-depth, mixed slashless+path entries, a ./-stays-root-only regression guard, and the bare-CSS+JS case.
|
CI on 4990da1 is green for the changes in this PR. The bundler DCE suite (including the five new tests) passed on all lanes. The remaining red lanes are unrelated to this diff:
This diff only touches |
…fects-no-slash-glob
An entry without "/" lists a file in every directory of the package, as in webpack and esbuild. Every entry was joined onto the package directory, so "*.side.js" and "polyfill.js" listed root files only and `bun build` left the nested files out of the bundle. A plain file name goes in a set keyed by file name, which is one hash lookup for each resolved file at any list size. A glob without "/" is stored as "**/<entry>", with no directory in the pattern. An entry with "/" or "\" is stored as before, and no list changes its variant. One function reads the array now. It replaces three copies of the join, two copies of the glob test and two copies of the ".css" skip. A list that has no pattern left to match no longer copies the path on each lookup.
The exact half and the glob half of `has_side_effects` each have one call site now, so both are inlined into the lookup as they were before this branch. The file-name probe stays out of line. A lookup in a list of paths takes 155 instructions in a release build, down from 171 with the two call sites. It takes 140 on main.
|
Status
|
A file name is now one more key of the exact set, with two flags that say which kinds of key the set holds. The separate boxed set and its copy of the package directory are gone, and so is the check that a named file is inside the package directory. An entry without "/" has no directory: a name and a glob now give the same answer for a file that "main" reaches outside the package, and both keep it. `SideEffects` is 80 bytes and `PackageJSON` is 632 bytes again, as on main, and a list with a file name allocates what it did on main. Tests: the CPU-time case is removed. It ran close to the default timeout on a debug build. New cases reach a listed file through a package that has a name, through the project's own package.json, through a barrel re-export, through "main" outside the package and through an onLoad plugin.
The exact probe and the glob loop are inlined into each arm of `has_side_effects`, as they were before this branch. The probe for a file name stays a call. Release build, instructions for one lookup of a file that the list does not name: no field 28 on main and 29 here, a list of paths 140 and 149, a glob with "/" 1,411 and 1,407.
|
Updated 4:36 AM PT - Oct 24th, 2026
✅ @robobun, your commit af8b80f8c57ceed2c7003bce9d156b846060bcd3 passed in 🧪 To try this PR locally: bunx bun-pr 36119That installs a local version of the PR into your bun-36119 --bun |
robobun
changed the title
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked two refactor-safety concerns in from_array and ruled them out: reusing the spill buffer across r_fs.join calls is safe because StringHashMapUnownedKey stores only hash+len, not the bytes (same as the old code); and replacing normalize_path_for_glob(..).unwrap_or_else(..) with a direct slashes_to_posix_in_place is behavior-preserving since that helper is infallible. The empty-glob-list short-circuit in any_glob_matches also matches the old loop's result.
Extended reasoning...
The diff reworks sideEffects array parsing in src/resolver/package_json.rs into SideEffects::from_array with a new ExactEntries set and **/-prefixed globs for slashless entries, plus new dce and barrel tests. No security-sensitive surface. Three confirmed findings are posted inline; this note records the refactor checks that were verified as behavior-preserving.
Problem
bun builddrops a file that a"sideEffects"entry without/lists below the package root.import "@fluentui/web-components/button/define-async.js"builds to 0 bytes, exit 0. Bun 1.2.20 kept it. Implement wildcard sideEffects support using glob API #21039 changed that.PackageJSON::parse(src/resolver/package_json.rs) joins every entry onto the package directory.Fix
/lists a file in every directory, as in webpack and esbuild: a file name as one more key of the exact set, a glob as**/<entry>.resolver.rsdo not change.test/bundler/esbuild/dce.test.ts(13 of 17 new cases fail on main),bundler_barrel.test.ts(2 of 2) andextra/TreeShaking8.finalize_resultguard of resolver: record enclosing package.json regardless of "name" field #33883.Background
"sideEffects"lists the files a bundler must keep when nothing uses their exports. The resolver asks it about every resolved file. A path or a file name costs one hash lookup, a glob one match.bun buildfrom 0.1 s to 5 s of CPU.Downsides
"x.js"and has anotherx.jsin a subdirectory now keeps both.import "pk"drops a listedmodulefile when the package also hasmain(resolver: record enclosing package.json regardless of "name" field #33883). An entry with/under a[slug]directory reads that name as glob syntax (Do not read the directory of a sideEffects glob or a --filter path as glob syntax #44939).Notes
The rule. webpack's tree-shaking guide: a pattern that does not include
/is treated like**/<pattern>(SideEffectsFlagPlugin.js,globToRegexp). esbuild does the same when it parses the package.json (internal/resolver/package_json.go). Before #21039 (first release 1.2.21) a list with a wildcard made Bun warn and keep the whole package. Since then a wildcard is a glob joined onto the package root.Published packages, run here. Release builds of main
5c6d4e24cdand of this branch, and esbuild 0.21.5.@fluentui/web-components@3.1.3, entryimport "@fluentui/web-components/button/define-async.js", its 4 dependencies external. The file isdist/esm/button/define-async.jsand callsButton.define(...). Bun 1.2.20: 8,767 bytes. Bun 1.2.21 and main: 0 bytes, exit 0. This branch: 8,520 bytes. esbuild: 10,644 bytes.@rustable/commons@0.4.12,"sideEffects": ["index.mjs"], entryimport "@rustable/commons",tslibexternal. The file islib/index.mjs. main: 0 bytes, exit 0. This branch: 40,328 bytes. esbuild: 67,186 bytes.What is a file name. An entry with no
/, no\and no glob syntax. Its key is the entry as written, in the same set as the path keys. Two flags say which kinds of key the set holds, so a list of paths does not hash the file name and a list of names does not hash the path. An entry with\stays a path, as on main.What a glob without
/becomes.**/<entry>, matched against the whole path. The pattern holds no directory, so a package under a directory named[slug]or{a,b}keeps its files (the joined form reads that name as glob syntax).["*"]and["**"]list every file, as in webpack and esbuild.A file outside the package directory.
"main": "../shared/x.js"makes the resolver ask this list about a file outside the package. An entry without/has no directory, so["x.js"]and["x.*"]both keep that file. main drops it. esbuild keeps it.dce/PackageJsonSideEffectsNoSlashOutsidePackagepins it.The
.cssskip. A list that has a glob does not store its.cssentries, and a list without a glob stores them. That stays, stated once. CSS is in the bundle either way. The flag changes the emitted JS only: a lazily loaded importer keeps an emptyinit_*for a CSS file that has side effects.dce/PackageJsonSideEffectsCSSEntriespins the three cases.Measured. Release builds of main and of this branch, same toolchain and flags.
perf,valgrind,bloatyandstraceare not on the machine. An instruction count is a gdbstepicount on one thread from the call of the named function to its return, with everything it calls. A call count is a gdb breakpoint hit count.Instructions per lookup (
primary_side_effects), steady state, forpkg/deep/er/file.jsthat no entry names:false["./polyfill.js"]["polyfill.js"]["./dist/*.side.js"]["./polyfill.js", "./dist/*.side.js"]./mN.j?["*.side.js"]["polyfill.js", "*.side.js"]["*.css"]finalize_resultfor one resolve: no field 1,162 -> 1,164,false1,130 -> 1,129,["./polyfill.js"]1,236 -> 1,245,["polyfill.js"]1,236 -> 1,296,["./dist/*.side.js"]3,928 -> 3,920,["*.css"]1,870 -> 1,186.primary_side_effectscalls per resolve are equal on both builds: no field 2,false1, paths 1, file names 1, globs 2, mixed 2.Bun.resolveSynccalls, allmi_*allocentry points): no field,false, paths and file names 1.0 -> 1.0. Glob and mixed lists 3.0 -> 3.0.["*.css"]3.0 -> 1.0, because a list with no pattern left returns before the path copy.dir_info_uncachedcall) are equal for 12 list shapes, a list of 1, 10 and 8,000 file names included.["*.css"]: 19 -> 18.size_of::<PackageJSON>(): 632 -> 632 bytes.size_of::<SideEffects>(): 80 -> 80 (-Zprint-type-sizes, release). The glob list is a boxed slice now, which makes room for the two flags.size), 11,008 fewer. The array arm compiles once, not once for each instantiation ofparse.m0.jsbun build e.jsbun e.js./m0.jsbun build e.js./m0.j?bun build e.jsbun e.js**/m0.jstakebun build e.js5,139, 5,051 and 5,396 ms.bun e.js: 4,699, 4,295 and 6,259 ms. The report measured the first version of this PR itself: 18 to 27 s against 0.17 s.Answers that change. Both release builds bundled a bare import of probe files for 214 lists: the 198
sideEffectsarrays of 2,403 sampled npm packages, and 16 edge lists ("",.,..,*,**,!x, classes, braces, non-strings). Probe files come from each entry, at the root and in subdirectories. 4,395 (list, file) pairs. 0 files that main keeps are dropped. 0 of the 177 lists that store no entry without a separator change. 31 lists change, and all of their 150 changed pairs are files that are now kept.Tests. In
dce.test.ts, 13 cases fail on main: the reported list through the CLI and throughBun.build, a glob-only list, a name-only list, a mixed list,["*"], a.cssname beside a.jsname, a scoped package that has a name and is reached through its index, the project's own package.json,mainoutside the package, a listed.scssthat anonLoadplugin turns into JS, a package.json without a name under a root that saysfalse, and a package under[slug]. 4 cases pass on main and pin it: entries with/stay paths, entries with\, an exact list in a package.json without a name, and the three.csscases.bundler_barrel.test.tsgets the #40650 fixture with["effect.js"]and["effect.*"].extra/TreeShaking8andTreeShaking9lose their skip option, and the option is deleted. Debug build of the last commit:dce.test.ts104 pass,bundler_barrel.test.ts72 pass,extra.test.ts224 pass,resolve.test.ts107 pass,bundler_splitting.test.ts209 pass,packagejson.test.ts94 pass,css.test.ts57 pass,bundler_plugin.test.ts68 pass.bundler_edgecase.test.ts: 187 pass and 1 failure,edgecase/DeepImportDiamondDAG. It gives itsbun buildof 20,000 modules 60 s, and that build takes 80 s on the debug binary in my container. A gdb breakpoint shows that its fixture never calls the lookup.bun run rust:check-allpasses forx86_64-pc-windows-msvcandaarch64-apple-darwin. On Windows x64dce.test.tshas the same 104 passes at the last commit, so the Windows expectation ofdce/PackageJsonSideEffectsEntryWithBackslashIsAPathis a run result.No timing test. An earlier commit had a CPU-time case (20,000 file names against the same list as paths). It ran 3 s and more on a debug build, close to the default timeout, and the self-review saw it fail 7 of 1,900 runs on a release build.
dce/PackageJsonSideEffectsFileNameInPackageWithoutNamefails without a clock when a file name is stored as a glob.Left as it is.
finalize_resultruns for themodulepath and then for themainpath of one package, and the second answer replaces the first. Soimport "pk"drops a listedmodulefile, also when the entry is a path such as./dist/index.esm.js. The same on main. esbuild keeps the file. resolver: record enclosing package.json regardless of "name" field #33883 changes that loop.finalize_resultasks a second time forGlobandMixedlists, through the nearest package.json that has a name. For a package.json without a name, a named ancestor that saysfalsestill overrules a glob list. webpack and esbuild ask once.sideEffectsfield or a list with a glob. Project["setup.js"]: Bun 1.4.3-canary dropsunnamed/deep/setup.jsandunnamed/other/plain.js. esbuild 0.28.2 and webpack 5.112.2 keep what the package.json of the dependency says. By the code this branch keepsdeep/setup.jsthere, because a file name has no directory. resolver: record enclosing package.json regardless of "name" field #33883 removes the name guard.bun run,bun test,Bun.resolveSync) asks the list too, and nothing reads the answer there.[""]matches nothing. webpack and esbuild match every file.\and no glob syntax matches nothing on POSIX. esbuild keeps the file./. A glob without/has no directory to split.extra.test.ts. If it merges after this change, check thatextra/TreeShaking8is not a todo again.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/esbuild/dce.test.ts