diff --git a/mordant-baseline.toml b/mordant-baseline.toml index 14c56af9d263..81f3540ada87 100644 --- a/mordant-baseline.toml +++ b/mordant-baseline.toml @@ -2,7 +2,6 @@ "bare_bool_args:src/ast/lib.rs" = 1 [bun_bundler] -"bare_bool_args:src/bundler/Chunk.rs" = 1 "defaulted_failure:src/bundler/bundle_v2.rs" = 2 "narrowed_two_ways:src/bundler/bundle_v2.rs" = 1 diff --git a/src/bundler/Chunk.rs b/src/bundler/Chunk.rs index 63e6420e24bb..2010966be9d9 100644 --- a/src/bundler/Chunk.rs +++ b/src/bundler/Chunk.rs @@ -424,6 +424,52 @@ pub struct CodeResult { pub(crate) shifts: Vec, } +/// What the paths `code()` writes over a chunk's references to other outputs +/// are relative to. A public path makes them outdir-relative either way. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum ReferencePathStyle { + /// The directory of the chunk being emitted, as in esbuild. + ImporterRelative, + /// The outdir, wherever the emitting chunk lands (`bun build --compile`). + OutdirRelative, +} + +impl ReferencePathStyle { + /// An executable loads every chunk from one virtual root, except for the + /// browser chunks a server build emits for its HTML imports: those are + /// served over HTTP. + pub(crate) fn for_chunk(chunk: &Chunk, compile: bool) -> ReferencePathStyle { + if compile + && !chunk + .flags + .contains(Flags::IS_BROWSER_CHUNK_FROM_SERVER_BUILD) + { + ReferencePathStyle::OutdirRelative + } else { + ReferencePathStyle::ImporterRelative + } + } +} + +/// Whether `code()` records how far each path it writes moves the text after it +/// (`CodeResult::shifts`, which the chunk's source map is corrected with) and +/// appends the `//# debugId` comment. Only wanted for a chunk that gets a map. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum SourceMapShiftTracking { + Disabled, + Enabled, +} + +impl SourceMapShiftTracking { + pub(crate) fn for_source_map(source_map: options::SourceMapOption) -> SourceMapShiftTracking { + if source_map == options::SourceMapOption::None { + SourceMapShiftTracking::Disabled + } else { + SourceMapShiftTracking::Enabled + } + } +} + // We don't need an allocator vtable here yet. `()` is kept as a token for the // caller's `Option<&DynAlloc>` plumbing; the actual // allocation goes through `alloc_buf` (global mimalloc) regardless. Real @@ -549,13 +595,12 @@ impl IntermediateOutput { // Accept both `&mut usize` and // `Option<&mut usize>` so call sites spelled either way compile. display_size: impl Into>, - force_absolute_path: bool, - enable_source_map_shifts: bool, + reference_path_style: ReferencePathStyle, + shift_tracking: SourceMapShiftTracking, ) -> Result { let display_size: Option<&mut usize> = display_size.into(); - // switch (enable_source_map_shifts) { inline else => |b| ... } - if enable_source_map_shifts { - self.code_with_source_map_shifts::( + match shift_tracking { + SourceMapShiftTracking::Enabled => self.code_with_source_map_shifts::( allocator_to_use, parse_graph, linker_graph, @@ -563,11 +608,10 @@ impl IntermediateOutput { chunk, chunks, display_size, - force_absolute_path, + reference_path_style, None, - ) - } else { - self.code_with_source_map_shifts::( + ), + SourceMapShiftTracking::Disabled => self.code_with_source_map_shifts::( allocator_to_use, parse_graph, linker_graph, @@ -575,9 +619,9 @@ impl IntermediateOutput { chunk, chunks, display_size, - force_absolute_path, + reference_path_style, None, - ) + ), } } @@ -598,13 +642,13 @@ impl IntermediateOutput { // Accept both `&mut usize` and // `Option<&mut usize>` so call sites spelled either way compile. display_size: impl Into>, - force_absolute_path: bool, - enable_source_map_shifts: bool, + reference_path_style: ReferencePathStyle, + shift_tracking: SourceMapShiftTracking, standalone_chunk_contents: &[Option>], ) -> Result { let display_size: Option<&mut usize> = display_size.into(); - if enable_source_map_shifts { - self.code_with_source_map_shifts::( + match shift_tracking { + SourceMapShiftTracking::Enabled => self.code_with_source_map_shifts::( allocator_to_use, parse_graph, linker_graph, @@ -612,11 +656,10 @@ impl IntermediateOutput { chunk, chunks, display_size, - force_absolute_path, + reference_path_style, Some(standalone_chunk_contents), - ) - } else { - self.code_with_source_map_shifts::( + ), + SourceMapShiftTracking::Disabled => self.code_with_source_map_shifts::( allocator_to_use, parse_graph, linker_graph, @@ -624,9 +667,9 @@ impl IntermediateOutput { chunk, chunks, display_size, - force_absolute_path, + reference_path_style, Some(standalone_chunk_contents), - ) + ), } } @@ -641,7 +684,7 @@ impl IntermediateOutput { chunk: &Chunk, chunks: &[Chunk], display_size: Option<&mut usize>, - force_absolute_path: bool, + reference_path_style: ReferencePathStyle, standalone_chunk_contents: Option<&[Option>]>, ) -> Result { // `Graph.input_files` SoA accessors live in `Graph::InputFileColumns`; @@ -682,8 +725,9 @@ impl IntermediateOutput { // esbuild's `pathBetweenChunks`: with a public path configured, every // reference is `publicPath + outdir-relative path`. Importer-relative // paths would escape the prefix from chunks in subdirectories. - let use_outdir_relative_path = - from_chunk_dir.is_empty() || force_absolute_path || !import_prefix.is_empty(); + let use_outdir_relative_path = from_chunk_dir.is_empty() + || reference_path_style == ReferencePathStyle::OutdirRelative + || !import_prefix.is_empty(); let urls_for_css: &[&[u8]] = if standalone_chunk_contents.is_some() { graph.ast.items_url_for_css() diff --git a/src/bundler/linker_context/MetafileBuilder.rs b/src/bundler/linker_context/MetafileBuilder.rs index 436ffeb60f29..b3da0e12c36a 100644 --- a/src/bundler/linker_context/MetafileBuilder.rs +++ b/src/bundler/linker_context/MetafileBuilder.rs @@ -43,7 +43,7 @@ use bun_ast::ExportsKind; use bun_ast::ImportKind; use bun_ast::ImportRecordFlags; -use crate::chunk::Content as ChunkContent; +use crate::chunk::{Content as ChunkContent, ReferencePathStyle, SourceMapShiftTracking}; use crate::options::Loader; use crate::{Chunk, Index, LinkerContext}; @@ -446,9 +446,9 @@ pub(crate) fn generate(c: &mut LinkerContext, chunks: &mut [Chunk]) -> crate::Re b"", // no import prefix for metafile &chunks[0], chunks, - None, // no display size - false, // not force absolute path - false, // no source map shifts + None, // no display size + ReferencePathStyle::ImporterRelative, + SourceMapShiftTracking::Disabled, )?; Ok(code_result.buffer) diff --git a/src/bundler/linker_context/generateChunksInParallel.rs b/src/bundler/linker_context/generateChunksInParallel.rs index 536354800a08..81f52306020d 100644 --- a/src/bundler/linker_context/generateChunksInParallel.rs +++ b/src/bundler/linker_context/generateChunksInParallel.rs @@ -17,6 +17,7 @@ use crate::Index; use crate::analyze_transpiled_module; use crate::analyze_transpiled_module::StringIDExt as _; use crate::cheap_prefix_normalizer; +use crate::chunk::{ReferencePathStyle, SourceMapShiftTracking}; use crate::options; use crate::options::Loader; @@ -667,8 +668,8 @@ pub(crate) fn generate_chunks_in_parallel( &chunks[ci], chunks, &mut ds, - false, - sourcemap_option != SourceMapOption::None, + ReferencePathStyle::ImporterRelative, + SourceMapShiftTracking::for_source_map(sourcemap_option), &scc, )?; chunks[ci].intermediate_output = intermediate_output; @@ -899,29 +900,24 @@ pub(crate) fn generate_chunks_in_parallel( &chunks[chunk_index_in_chunks_list], chunks, &mut display_size, - false, - false, + ReferencePathStyle::ImporterRelative, + SourceMapShiftTracking::Disabled, standalone_chunk_contents.as_deref().unwrap(), )? } else { - let force_abs = c.resolver().opts.compile - && !chunks[chunk_index_in_chunks_list] - .flags - .contains(crate::chunk::Flags::IS_BROWSER_CHUNK_FROM_SERVER_BUILD); - let enable_sm = chunks[chunk_index_in_chunks_list] - .content - .sourcemap(c.options.source_maps) - != SourceMapOption::None; + let chunk = &chunks[chunk_index_in_chunks_list]; intermediate_output.code( None, c.parse_graph(), &c.graph, public_path, - &chunks[chunk_index_in_chunks_list], + chunk, chunks, &mut display_size, - force_abs, - enable_sm, + ReferencePathStyle::for_chunk(chunk, c.resolver().opts.compile), + SourceMapShiftTracking::for_source_map( + chunk.content.sourcemap(c.options.source_maps), + ), )? }; // Tail of the loop body needs `&mut chunk` (`output_source_map.finalize()`); diff --git a/src/bundler/linker_context/writeOutputFilesToDisk.rs b/src/bundler/linker_context/writeOutputFilesToDisk.rs index 4e3d991dde76..8be1e88e3f5d 100644 --- a/src/bundler/linker_context/writeOutputFilesToDisk.rs +++ b/src/bundler/linker_context/writeOutputFilesToDisk.rs @@ -10,7 +10,7 @@ use bun_paths::{self as paths, PathBuffer}; use bun_wyhash::hash; use crate::LinkerContext; -use crate::chunk::{Content, Flags as ChunkFlags}; +use crate::chunk::{Content, Flags as ChunkFlags, ReferencePathStyle, SourceMapShiftTracking}; use crate::linker_context::output_file_list_builder::OutputFileList; use crate::linker_context_mod::debug; use crate::options::{self, Loader, OutputFile, SourceMapOption}; @@ -247,8 +247,8 @@ pub(crate) fn write_output_files_to_disk( chunk, chunks, Some(&mut display_size), - false, - false, + ReferencePathStyle::ImporterRelative, + SourceMapShiftTracking::Disabled, scc, ) { Ok(r) => r, @@ -265,11 +265,10 @@ pub(crate) fn write_output_files_to_disk( chunk, chunks, Some(&mut display_size), - resolver_opts.compile - && !chunk - .flags - .contains(ChunkFlags::IS_BROWSER_CHUNK_FROM_SERVER_BUILD), - chunk.content.sourcemap(c.options.source_maps) != SourceMapOption::None, + ReferencePathStyle::for_chunk(chunk, resolver_opts.compile), + SourceMapShiftTracking::for_source_map( + chunk.content.sourcemap(c.options.source_maps), + ), ) { Ok(r) => r, Err(_e) => bun_core::Output::panic(format_args!( diff --git a/src/runtime/bake/DevServer.rs b/src/runtime/bake/DevServer.rs index e8f6e34786c5..dc9bbecd79b4 100644 --- a/src/runtime/bake/DevServer.rs +++ b/src/runtime/bake/DevServer.rs @@ -4027,8 +4027,8 @@ pub(super) fn finalize_bundle( // and `code()` only reads. unsafe { ::core::slice::from_raw_parts(chunks_ptr, chunks_len) }, None, - false, - false, + bundler::chunk::ReferencePathStyle::ImporterRelative, + bundler::chunk::SourceMapShiftTracking::Disabled, )? }; diff --git a/test/bake/dev/css.test.ts b/test/bake/dev/css.test.ts index 18d665708976..58f01938a773 100644 --- a/test/bake/dev/css.test.ts +++ b/test/bake/dev/css.test.ts @@ -181,6 +181,12 @@ devTest("asset referenced in css", { let backgroundImage = await c.style("body").backgroundImage; assert(backgroundImage); await dev.fetch(extractCssUrl(backgroundImage)).expectFile(imageFixtures.bun); + // The served stylesheet is the chunk with the asset reference resolved and + // nothing else: CSS never gets a source map, so no debugId trailer either. + const stylesheetHref = (await (await dev.fetch("/")).text()).match(/]*href="([^"]+)"/)![1]; + const stylesheet = await (await dev.fetch(stylesheetHref)).text(); + expect(stylesheet).toContain("background-image:"); + expect(stylesheet).not.toContain("debugId"); await dev.write("bun.png", imageFixtures.bun2); backgroundImage = await c.style("body").backgroundImage; assert(backgroundImage); diff --git a/test/bundler/bundler_splitting.test.ts b/test/bundler/bundler_splitting.test.ts index bf316b2d1377..f60088f59d86 100644 --- a/test/bundler/bundler_splitting.test.ts +++ b/test/bundler/bundler_splitting.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { bunEnv, bunExe, isASAN, isDebug, tempDir } from "harness"; import { readdirSync, readFileSync } from "node:fs"; import { join } from "node:path"; +import { SourceMapConsumer } from "source-map"; import { itBundled } from "./expectBundled"; const env = { @@ -402,4 +403,95 @@ describe("bundler", () => { } expect(runOut.trim()).toBe(`${(N * (N - 1)) / 2} 0 ${N - 1}`); }, 60_000); + + // Chunks are printed with placeholders where they refer to other chunks and + // assets; the placeholders are replaced once every output path is known. + // These pin the two per-chunk decisions of that step: what the written paths + // are relative to, and whether the chunk's source map is corrected for the + // replacements (which is also what adds the `//# debugId` comment). + describe.concurrent("splitting/ChunkReferencePaths", () => { + const adminSource = `import { util } from "../../shared/util.js";\nexport const page = [import("../site/index.js"), util("admin")];\n`; + const files = { + "pages/admin/index.js": adminSource, + "pages/site/index.js": `import { util } from "../../shared/util.js";\nexport const page = util("site");\n`, + "shared/util.js": `export function util(x) {\n return "util:" + x;\n}\n`, + }; + + async function buildAdminEntry( + root: string, + options: { sourcemap: "none" | "linked"; publicPath?: string; outdir?: string }, + ) { + const result = await Bun.build({ + entrypoints: [join(root, "pages/admin/index.js"), join(root, "pages/site/index.js")], + root, + splitting: true, + ...options, + }); + expect(result.logs).toBeEmpty(); + const admin = result.outputs.find(o => o.path.replaceAll("\\", "/").endsWith("pages/admin/index.js"))!; + const map = result.outputs.find(o => o.path === admin.path + ".map"); + return { code: await admin.text(), map: map && JSON.parse(await map.text()) }; + } + + // `util("admin")` follows the dynamic import on the same line, so the map + // only points at it if the mappings were shifted by the difference between + // the placeholder and the path written over it. + async function expectUtilCallToBeMapped(code: string, map: object) { + const generatedLines = code.split("\n"); + const line = generatedLines.findIndex(l => l.includes('util("admin")')) + 1; + expect(line).toBeGreaterThan(0); + const column = generatedLines[line - 1].indexOf('util("admin")'); + const original = await SourceMapConsumer.with(map, null, consumer => + consumer.originalPositionFor({ line, column }), + ); + expect({ + source: original.source?.replaceAll("\\", "/").split("/").slice(-3).join("/"), + line: original.line, + column: original.column, + }).toEqual({ + source: "pages/admin/index.js", + line: 2, + column: adminSource.split("\n")[1].indexOf('util("admin")'), + }); + } + + test("references are relative to the directory of the importing chunk", async () => { + using dir = tempDir("splitting-reference-paths", files); + const { code, map } = await buildAdminEntry(String(dir), { sourcemap: "none" }); + expect(code).toMatch(/from "\.\.\/\.\.\/chunk-[a-z0-9]+\.js"/); + expect(code).toContain('import("../site/index.js")'); + expect(code).not.toContain("//# debugId="); + expect(map).toBeUndefined(); + }); + + test("a public path makes references outdir-relative regardless of the importing chunk's directory", async () => { + using dir = tempDir("splitting-reference-paths-public", files); + const { code, map } = await buildAdminEntry(String(dir), { + sourcemap: "linked", + publicPath: "https://cdn.example/app/", + }); + expect(code).toMatch(/from "https:\/\/cdn\.example\/app\/chunk-[a-z0-9]+\.js"/); + expect(code).toContain('import("https://cdn.example/app/pages/site/index.js")'); + expect(code).toContain("//# debugId="); + await expectUtilCallToBeMapped(code, map); + }); + + test("the source map accounts for the paths written over the placeholders", async () => { + using dir = tempDir("splitting-reference-paths-sourcemap", files); + const { code, map } = await buildAdminEntry(String(dir), { sourcemap: "linked" }); + expect(code).toContain('import("../site/index.js")'); + expect(code).toContain("//# debugId="); + await expectUtilCallToBeMapped(code, map); + }); + + test("writing to an outdir resolves references the same way as an in-memory build", async () => { + using dir = tempDir("splitting-reference-paths-outdir", files); + const inMemory = await buildAdminEntry(String(dir), { sourcemap: "linked" }); + const onDisk = await buildAdminEntry(String(dir), { sourcemap: "linked", outdir: join(String(dir), "out") }); + expect(onDisk.code).toBe(inMemory.code); + expect(onDisk.map.mappings).toBe(inMemory.map.mappings); + expect(readFileSync(join(String(dir), "out", "pages", "admin", "index.js"), "utf8")).toBe(inMemory.code); + await expectUtilCallToBeMapped(onDisk.code, onDisk.map); + }); + }); }); diff --git a/test/bundler/standalone.test.ts b/test/bundler/standalone.test.ts index 2813ee97c981..2fb0545b9b29 100644 --- a/test/bundler/standalone.test.ts +++ b/test/bundler/standalone.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import { bunEnv, bunExe, tempDir } from "harness"; import { existsSync } from "node:fs"; +import { SourceMapConsumer } from "source-map"; describe("compile --target=browser", () => { test("inlines JS and CSS into HTML", async () => { @@ -658,5 +659,77 @@ console.log(greet("world"));`, const html = await result.outputs[0].text(); expect(html).toContain("//# sourceMappingURL=data:application/json;base64,"); }); + + // Standalone HTML is assembled in two passes: each script and stylesheet + // chunk is resolved on its own first (asset references become data: URIs, + // and when a source map is being emitted the chunk's map is corrected for + // them and the chunk gets a debugId), then the HTML chunk inlines the + // results. The document itself never has a source map, so nothing may be + // appended to it. + const assetFixture = { + "index.html": `\n\n\n\n\n\n`, + "app.js": `import pic from "./pic.svg";\nexport function greet(name) {\n return name + pic;\n}\nconsole.log(greet("x"));\n`, + "pic.svg": `\n`, + }; + + async function buildWithAsset(dir: string, options: { sourcemap: "none" | "linked"; outdir?: string }) { + const result = await Bun.build({ + entrypoints: [`${dir}/index.html`], + compile: true, + target: "browser", + // Puts the whole script on one line, so the data: URI written over the + // asset placeholder shifts the columns of everything after it. + minify: { whitespace: true }, + ...options, + }); + expect(result.logs).toBeEmpty(); + const html = await result.outputs.find(o => o.loader === "html")!.text(); + const map = result.outputs.find(o => o.kind === "sourcemap"); + return { html, map: map && JSON.parse(await map.text()) }; + } + + async function expectInlinedScriptToBeMapped(html: string, map: object) { + const open = '")); + const [firstLine] = script.split("\n"); + const column = firstLine.indexOf("function greet"); + expect(column).toBeGreaterThan(firstLine.indexOf('"data:image/svg+xml')); + const original = await SourceMapConsumer.with(map, null, consumer => + consumer.originalPositionFor({ line: 1, column }), + ); + expect({ line: original.line, column: original.column }).toEqual({ + line: 2, + column: assetFixture["app.js"].split("\n")[1].indexOf("function greet"), + }); + } + + test("without a sourcemap option, nothing source map related is written into the document", async () => { + using dir = tempDir("compile-browser-asset-no-sourcemap", assetFixture); + const { html, map } = await buildWithAsset(String(dir), { sourcemap: "none" }); + expect(map).toBeUndefined(); + expect(html).toContain('"data:image/svg+xml'); + expect(html).not.toContain("debugId"); + expect(html).not.toContain("sourceMappingURL"); + expect(html).toEndWith("\n"); + }); + + test("the inlined script's map accounts for the data: URI written over its asset import", async () => { + using dir = tempDir("compile-browser-asset-sourcemap", assetFixture); + const { html, map } = await buildWithAsset(String(dir), { sourcemap: "linked" }); + // The script carries the debugId; the document around it gets nothing appended. + expect(html.match(/\/\/# debugId=/g)).toHaveLength(1); + expect(html.indexOf("//# debugId=")).toBeLessThan(html.indexOf("")); + expect(html).toEndWith("\n"); + await expectInlinedScriptToBeMapped(html, map); + }); + + test("writing to an outdir inlines and maps the script the same way as an in-memory build", async () => { + using dir = tempDir("compile-browser-asset-sourcemap-outdir", assetFixture); + const { html, map } = await buildWithAsset(String(dir), { sourcemap: "linked", outdir: `${dir}/dist` }); + expect(await Bun.file(`${dir}/dist/index.html`).text()).toBe(html); + expect(html.match(/\/\/# debugId=/g)).toHaveLength(1); + expect(html).toEndWith("\n"); + await expectInlinedScriptToBeMapped(html, map); + }); }); });