Repository navigation
Conversation
…ndex
The metafile wrote `"external": true` for an import when its record had
IS_EXTERNAL_WITHOUT_SIDE_EFFECTS or no source index. The flag is a tree
shaking hint ("this import can be removed if it's unused"). An embedded
sqlite import (`with { type: "sqlite", embed: "true" }`) gets the flag
before resolution and then resolves to a bundled file, so the metafile
marked a bundled input as external.
The test is now the source index alone. esbuild uses that test for
`inputs`, and the `--compile --aot` metafile already follows that rule.
A record with no source index keeps `"external": true`.
|
Updated 7:55 AM PT - Oct 28th, 2026
❌ @robobun, your commit fa2d86c has 7 failures in
🧪 To try this PR locally: bunx bun-pr 47161That installs a local version of the PR into your bun-47161 --bun |
|
Status: ready for review. The tests of this change pass in CI. The red steps are tests that this PR cannot reach. How I reproduced it. printf x > app.db
printf 'import db from "./app.db" with { type: "sqlite", embed: "true" };\nconsole.log(db);\n' > index.js
bun build index.js --target=bun --outdir=dist --metafile=meta.json
jq -c '.inputs["index.js"].imports, (.inputs | keys)' meta.jsonOn main (11c3671) and on bun 1.4.3-canary.1 the import entry is
CI. Build 134382 (fa2d86c):
PR: #47161 |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
External-import classification and coverage src/bundler/linker_context/MetafileBuilder.rs, test/bundler/metafile.test.ts |
The linker no longer marks imports with a valid source index as external based on IS_EXTERNAL_WITHOUT_SIDE_EFFECTS. Tests cover embedded SQLite imports, excluded and tree-shaken databases, split dynamic imports, and CLI metafile output. |
Suggested reviewers: alii
Priority:
Merge Risk: 
fa2d8
-
The change is consistent with the intended metafile behavior and adds coverage for the relevant embedded and external import cases.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: metafile imports are marked external only when they lack a valid source index. Description check The description is detailed and covers the change, cause, fix, verification, tests, scope, and known limitations. It does not use the template headings, but it includes the required information and is… 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. Full details: Description check
-
Explanation
The description is detailed and covers the change, cause, fix, verification, tests, scope, and known limitations. It does not use the template headings, but it includes the required information and is substantially complete.
-
- 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.
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 the sibling consumers of IS_EXTERNAL_WITHOUT_SIDE_EFFECTS and the other metafile writers: the markdown writer derives external from the JSON external key (import_target in MetafileBuilder.rs), so the [EXTERNAL:]/[IMPORT:] lines and the "External imports" count follow the new condition without a separate change; the --compile --aot writer in src/bundler/aot/metafile.rs already keys external on ImportTarget::External only, so the two writers now agree on importer entries; and the remaining flag readers (mergeSmallChunks.rs, generateCompileResultForHtmlChunk.rs, LinkerContext.rs) are tree-shaking/HTML consumers, not metafile output.
Extended reasoning...
The change is a one-condition narrowing in src/bundler/linker_context/MetafileBuilder.rs plus new tests in test/bundler/metafile.test.ts; it touches no security-sensitive surface. The inline findings (parse-order-dependent external for a mixed embed/non-embed absolute import, two weak assertions, discarded stdout in the CLI test) are the reasons a human should look; the note above records only the sibling-site checks that were ruled out.
The test for an unused embedded import compares the whole entry. The test for a plain sqlite import beside an embedded one compares `external` of both entries with the output. The CLI test checks that the build lists the copied database on stdout.
There was a problem hiding this comment.
LGTM — the follow-up commit pins the exact metafile entries and the CLI stdout, and the one-line condition change checks out.
What was reviewed:
- Traced every bundler site that sets
IS_EXTERNAL_WITHOUT_SIDE_EFFECTS(bundle_v2.rs 6745/6792/6807/6818/7178): each either setssource_index = INVALIDorcontinues before resolution, so builtins,bun:specifiers, plaintype: "sqlite"and resolver-externals still emitexternal: true; only resolvedSqliteEmbeddedrecords change. - Confirmed the linker (LinkerContext.rs:3188) reads the flag only when
source_indexis invalid, so leaving the flag on embedded records is harmless and the metafile was the sole consumer that read it on a resolved record. - Checked
bundler_bun.test.ts(UnknownBunSpecifier metafile assertion) still holds under the new condition;ImportRecordFlagsremains used at MetafileBuilder.rs:352 so no dead import.
Extended reasoning...
The change narrows the external condition in src/bundler/linker_context/MetafileBuilder.rs:364 from a flag-or-no-source-index test to just no-source-index, plus about 200 lines of new tests in test/bundler/metafile.test.ts covering the embedded sqlite routes (static, sqlite_embedded, dynamic with and without splitting, onResolve variants, files map, shared importers, unused, external option, plain sqlite, CLI --metafile and --metafile-md). It touches no security-sensitive surface; it only affects a diagnostic JSON field. The second commit addressed three of the four test-shape nits from the prior review (exact entry for the unused case, stdout assertion in the CLI test, a comment on the plugin-path fixture), and the fourth was flagged as pre-existing and non-blocking. The Rust change is one line, verified behavior-preserving for every other producer of the flag, no CODEOWNER covers the paths, and the hunt ran dry with no findings.
Problem
bun build --metafilemarksimport db from "./app.db" with { type: "sqlite", embed: "true" }as"external": trueininputs[..].imports. The same metafile givesapp.db91 bytes of the chunk, and--compile --aot --metafileprints the entry withoutexternal.externaltest ingenerate(src/bundler/linker_context/MetafileBuilder.rs:364) isflags.contains(IS_EXTERNAL_WITHOUT_SIDE_EFFECTS) || !source_index.is_valid().resolve_import_records(src/bundler/bundle_v2.rs:6822) sets that flag on an embedded import before it resolves to a bundled file.Fix
!record.source_index.is_valid(), which esbuild uses forinputs(internal/bundler/bundler.go:2359, v0.25.1). A record with no source index keeps"external": true.test/bundler/metafile.test.ts(10 of 14 new tests fail without the change). Alsotest/bundler/esbuild/metafile.test.ts,bundler_bun.test.ts.Background
source_indexnames the bundled file an import resolves to.IS_EXTERNAL_WITHOUT_SIDE_EFFECTSis a tree shaking hint: "this import can be removed if it's unused" (src/ast/import_record.rs:76). The linker reads it only when the record has no source index.Downsides
external: truemust readinputs. An unused import'spathis not a key there.externalfollows it. That binding depends on the parse order (bundler: link an import record through the path map only when resolution pointed it at a module #45248)..textshrinks 256 bytes. No emitted file changes in 24 builds.Notes
Repro
Before,
meta.jsonhas all of these:inputs["index.js"].imports:[{"path":"app.db","kind":"import-statement","original":"./app.db","external":true}]inputs["app.db"]is a key.outputs["./index.js"].inputs["app.db"].bytesInOutput: 91Before,
meta.mdlistsapp.dbat91 bytes | 65.0%of the output. It also prints| External imports | 1 |,[EXTERNAL: index.js imports app.db]andImported by: (entry point or orphan)for it. With the change it prints[IMPORT: index.js -> app.db]and[IMPORTED_BY: app.db <- index.js].History
bun build --compile --aot#45631 added a second metafile writer for--compile --aot(src/bundler/aot/metafile.rs:140). It writesexternalonly for an import with no input. Its test file says that its inputs are those of the same build without--aot. With this change the two writers agree on the entry of the importer. They still differ on the entry of the database itself.What changes
fileimport prints. Each route has a test: a static import,type: "sqlite_embedded",import()without splitting, anonResolveplugin that returns a path, anonResolveplugin that returns nothing, thefilesmap, two importers of one database, and the CLI (--metafile,--metafile-md).--compilecalls the samegenerate(checked by hand). With a plugin path, the loader comes from the extension of that path, so thefileloader copies the database. The record still has the flag from the attribute.inputsdoes not list a dropped file (bundler: don't emit chunks for import() targets referenced only from dead code #39591,test/bundler/bundler_splitting.test.ts:383). The database is still copied to the output directory. The entry wasexternal: true. It is now an entry whosepathis not a key ofinputs, which is what an unusedfileimport prints on main. A reader that follows such an entry intoinputswithout a check fails, on main too. bundler: list tree-shaken files in the metafile inputs so import edges resolve #38624 proposes to list such files. The test for this case compares the whole entry and does not checkinputs.patch_import_record_source_indicesbinds by path text (src/bundler/bundle_v2.rs:7563). That lookup can give a source index to a record that did not resolve to a module, for exampleimport b from "/app.db" with { type: "sqlite" }next to an embedded import of the same file. The output then reads the embedded module for both imports. The entry wasexternal: true. It now has noexternal, in agreement with the output. For two sibling files, the lookup result depends on the parse order, so the output differs between two builds of one input, on main too. In 80 builds of such a case, main printedexternal: truein all 80, and the output kept the import in 60. This branch printedexternal: truein 66 of 80, the same 66 whose output kept the import. bundler: link an import record through the path map only when resolution pointed it at a module #45248 removes the lookup for such records. The test for this case checks thatexternalagrees with the output.What keeps
external: true, each with a test (these 4 pass with and without the change):with { type: "sqlite" }withoutembed, an embedded import underexternal: ["*.db"], an embedded import that anonResolveplugin marks external, and a splitimport()(the entry keepsentryPointandexternal: true, as for each split import).Not changed here
resolve_import_records.generateCompileResultForHtmlChunk.rs:151reads the flag with no source index test. HTML tags carry no import attributes, so an embedded sqlite record does not reach it.outputs[..].importslists imports to other chunks only (packages/bun-types/bun.d.ts:4805). esbuild also lists external imports there. That is a separate feature. The self-review asked for a tracking issue for it. None is filed: no user asked for it.external: true. esbuild prints the same entry (bundler.go:505leaves it with no resolve result).inputs["app.db"].bytesis 0 and the copied database is not inoutputs. bundler: read the metafile size of a copied input from its asset file #38341 and fix(bundler): include emitted files in metafile outputs #43823 are open for those fields.Measurements
Base is the same commit with the hunk reverted. Both are release builds (ThinLTO, linux x64).
.text,metafile_builder::generate9,246 -> 9,230 bytes (nm -S). No other symbol of 182,037 changes size. The stripped binary stays at 138,197,144 bytes.externalpush (objdump). Thetest/jneon flag bit 11 is gone.generate(gdb breakpoint, 403 modules).resolve_import_records(13,725 bytes) andpatch_import_record_source_indices(1,613 bytes) symbol sizes equal: yes.metafile_builder::generateon an 801-import graph: 251,462 -> 249,859. Counted with gdb single-step, callees skipped. valgrind (cachegrind, dhat) cannot be installed in the build container.externaloption,filesmap,import()with and without splitting, plain sqlite,sqlite_embedded, and a control with builtins, each with the binding used and unused.external: 16 across 24 builds. Edges with no source index changed: 0 of 19. No other metafile field differs.strace -fc, 3 runs each). 8 kinds such asfutexvary between runs of one build. Allocator calls insidegenerate: 12 malloc, 18 realloc, 26 free in both (gdb breakpoint counts).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/metafile.test.ts