Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions src/bundler/bundle_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3911,10 +3911,10 @@ pub mod bv2_impl {
// sidestep for the `&mut self` overlap.
this.enqueue_entry_points_normal(unsafe { &*entry_points })?;

if this.transpiler.log().has_errors() {
return Err(crate::Error::BuildFailed);
}

// Entry point errors are reported after the pool drains: the
// runtime parse is already scheduled, and tearing the workers
// down while one is still setting itself up reads a
// half-initialized `Worker` out of `workers_assignments`.
Comment thread
robobun marked this conversation as resolved.
Outdated
this.wait_for_parse();
this.dump_pool_stats("parse");

Expand Down Expand Up @@ -4085,10 +4085,7 @@ pub mod bv2_impl {

this.enqueue_entry_points_bake_production(entry_points)?;

if this.transpiler.log().has_errors() {
return Err(crate::Error::BuildFailed);
}

// No early return before the drain; see `generate_from_cli`.
this.wait_for_parse();

if this.transpiler.log().has_errors() {
Expand Down Expand Up @@ -4862,6 +4859,27 @@ pub mod bv2_impl {
} else {
drop(result.namespace);
drop(result.path);

// An external entry point has nothing to bundle; the build
// drivers only find out about a dropped entry point from
// the log. Same error as esbuild.
Comment thread
robobun marked this conversation as resolved.
Outdated
if resolve.import_record.kind == ImportKind::EntryPointBuild {
// Entry points have no importer (`source_file` is empty);
// the dev server keys the failure by the entry point's
// own path, which is the specifier.
Comment thread
robobun marked this conversation as resolved.
Outdated
this.log_for_resolution_failures(
&resolve.import_record.specifier,
resolve.import_record.original_target.bake_graph(),
)
.add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!(
"The entry point {} cannot be marked as external",
bun_core::fmt::quote(&resolve.import_record.specifier),
),
);
}
}

if let Some(source_index) = out_source_index {
Expand Down
47 changes: 45 additions & 2 deletions src/bundler/transpiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,15 @@

/// Resolve an entry-point specifier, busting the directory cache and
/// retrying once on failure before reporting the error to the log.
///
/// Every failure is logged before it is returned: callers drop the entry
/// point on `Err`, and the build drivers decide whether to keep going from
/// the log alone. That includes an entry point the resolver disabled (mapped
/// to `false` by a package.json `"browser"` field, or a Node.js builtin that
/// browser builds stub out), which has no module to bundle.
pub fn resolve_entry_point(&mut self, entry_point: &[u8]) -> crate::Result<resolver::Result> {
match self._resolve_entry_point(entry_point) {
Ok(r) => Ok(r),
Ok(r) => self.reject_disabled_entry_point(r, entry_point),

Check warning on line 467 in src/bundler/transpiler.rs

View check run for this annotation

Claude / Claude Code Review

Too-long-path guard arm returns Err without logging, contradicting the new 'every failure is logged' contract

The new doc comment states "Every failure is logged before it is returned", but the pre-existing guard arm just below (`top_level_dir.len() + entry_point.len() + 4 > MAX_PATH_BYTES`, lines 470-475) returns `Err(err)` without calling `add_error_fmt` — so a >~4090-byte entry-point specifier that fails to resolve is still silently dropped, the same bug class this PR closes. Consider logging before returning there too (mirroring the general `Err` arm at line 529), or at minimum softening the doc com
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
// Nothing that long names a directory whose cache could be stale
// (and the join below has a PathBuffer to fit `top_level_dir/entry/..` in).
Err(err)
Expand Down Expand Up @@ -515,7 +521,7 @@
// Only re-query if we previously had something cached.
if busted {
if let Ok(result) = self._resolve_entry_point(entry_point) {
return Ok(result);
return self.reject_disabled_entry_point(result, entry_point);
}
// ignore this error, we will print the original error
}
Expand All @@ -534,6 +540,43 @@
}
}

/// A resolver result with no usable path is a module the resolver disabled
/// (`Result::path` skips disabled paths). Imports of such a module become an
/// empty object, but an entry point has nothing to produce, so report it
/// like any other entry point that fails to resolve.
Comment thread
robobun marked this conversation as resolved.
Outdated
fn reject_disabled_entry_point(
&self,
resolved: resolver::Result,
entry_point: &[u8],
) -> crate::Result<resolver::Result> {
if resolved.path_const().is_some() {
return Ok(resolved);
}

// The resolver gives builtins it stubs out for the browser the "node"
// namespace; everything else disabled comes from a "browser" map.
Comment thread
robobun marked this conversation as resolved.
Outdated
if resolved.path_pair.primary.namespace == b"node" {
self.log_mut().add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!(
"Browser build cannot use Node.js builtin \"{}\" as an entry point. To use Node.js builtins, set target to 'node' or 'bun'",
bstr::BStr::new(entry_point)
),
);
} else {
self.log_mut().add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!(
"\"{}\" is disabled due to \"browser\" field in package.json (entry point)",
bstr::BStr::new(entry_point)
),
);
}
Err(crate::Error::ResolveMessage)
}

/// Load env files and build `options.define`. Idempotent — a no-op once
/// `options.defines_loaded` is set.
pub fn configure_defines(&mut self) -> crate::Result<()> {
Expand Down
52 changes: 52 additions & 0 deletions test/bundler/bun-build-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,58 @@ describe("Bun.build", () => {
}
});

// Runs in a child because the unfixed behavior was a process abort: the
// disabled entry point was dropped without an error and the linker ran with
// zero entry points.
test.concurrent("an entry point disabled by the package.json browser field is a build error", async () => {
using dir = tempDir("build-entry-point-disabled-by-browser-field", {
"package.json": JSON.stringify({ name: "app", browser: { "./entry.js": false } }),
"entry.js": `console.log("entry");`,
"build.mjs": `
const returned = await Bun.build({ entrypoints: ["./entry.js"], target: "browser", throw: false });
let thrown;
try {
await Bun.build({ entrypoints: ["./entry.js"], target: "browser" });
} catch (e) {
thrown = {
isAggregateError: e instanceof AggregateError,
errors: e.errors.map(error => ({ name: error.name, level: error.level, position: error.position, message: error.message })),
};
}
console.log(JSON.stringify({
returned: {
success: returned.success,
outputs: returned.outputs.length,
logs: returned.logs.map(log => ({ name: log.name, level: log.level, position: log.position, message: log.message })),
},
thrown,
}));
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "build.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

const message = {
name: "BuildMessage",
level: "error",
position: null,
message: '"./entry.js" is disabled due to "browser" field in package.json (entry point)',
};
expect(stderr).toBe("");
expect(JSON.parse(stdout)).toEqual({
returned: { success: false, outputs: 0, logs: [message] },
thrown: { isAggregateError: true, errors: [message] },
});
expect(exitCode).toBe(0);
});

test("returns output files", async () => {
Bun.gc(true);
const build = await Bun.build({
Expand Down
81 changes: 81 additions & 0 deletions test/bundler/bundler_browser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,87 @@ describe("bundler", () => {
},
});

// An entry point the "browser" field maps to false has nothing to bundle.
// This used to reach the linker with zero entry points and crash
// ("index out of bounds" in generateChunksInParallel); with a second, live
// entry point it silently built only that one.
const browserFieldDisabledEntryPointFiles = {
"/package.json": /* json */ `
{ "name": "app", "browser": { "./entry.js": false } }
`,
"/entry.js": /* js */ `
console.log("entry");
`,
"/other.js": /* js */ `
console.log("other");
`,
};
itBundled("browser/EntryPointDisabledByBrowserField", {
skipOnEsbuild: true,
backend: "cli",
files: browserFieldDisabledEntryPointFiles,
entryPointsRaw: ["./entry.js"],
target: "browser",
bundleErrors: {
"<bun>": ['"./entry.js" is disabled due to "browser" field in package.json (entry point)'],
},
});
itBundled("browser/EntryPointDisabledByBrowserFieldNextToLiveEntryPoint", {
skipOnEsbuild: true,
backend: "cli",
files: browserFieldDisabledEntryPointFiles,
entryPointsRaw: ["./entry.js", "./other.js"],
target: "browser",
bundleErrors: {
"<bun>": ['"./entry.js" is disabled due to "browser" field in package.json (entry point)'],
},
});
itBundled("browser/EntryPointDisabledByBrowserFieldOnlyAppliesToBrowserTarget", {
skipOnEsbuild: true,
backend: "cli",
files: browserFieldDisabledEntryPointFiles,
entryPointsRaw: ["./entry.js"],
target: "bun",
run: {
file: "/out/entry.js",
stdout: "entry",
},
});
itBundled("browser/EntryPointDisabledByPackageMainBrowserField", {
// The disabled module is reached through a package's "main", so the entry
// point specifier and the disabled file differ.
skipOnEsbuild: true,
backend: "cli",
files: {
"/node_modules/pkg/package.json": /* json */ `
{ "name": "pkg", "main": "./node.js", "browser": { "./node.js": false } }
`,
"/node_modules/pkg/node.js": /* js */ `
console.log("node only");
`,
},
entryPointsRaw: ["pkg"],
target: "browser",
bundleErrors: {
"<bun>": ['"pkg" is disabled due to "browser" field in package.json (entry point)'],
},
});
itBundled("browser/EntryPointIsNodeBuiltinStubbedForBrowser", {
// Browser builds replace "fs" (and node:* builtins without a polyfill) with
// an empty module, so as entry points they have nothing to bundle either.
skipOnEsbuild: true,
backend: "cli",
files: {},
entryPointsRaw: ["fs", "node:fs"],
target: "browser",
bundleErrors: {
"<bun>": [
`Browser build cannot use Node.js builtin "fs" as an entry point. To use Node.js builtins, set target to 'node' or 'bun'`,
`Browser build cannot use Node.js builtin "node:fs" as an entry point. To use Node.js builtins, set target to 'node' or 'bun'`,
],
},
});

// unsure: do we want polyfills or no-op stuff like node:* has
// right now all error except bun:wrap which errors at resolve time, but is included if external
const bunModules: Record<string, "no-op" | "polyfill" | "error"> = {
Expand Down
78 changes: 78 additions & 0 deletions test/bundler/bundler_plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1688,4 +1688,82 @@ describe("bundler", () => {
expect(exitCode).toBe(0);
});
}

// An entry point that an onResolve plugin leaves without a module (marks it
// external, or declines it and the package.json "browser" field disables it)
// must fail the build. It used to be dropped without a log entry, which
// crashed the linker when it was the only entry point and silently produced
// fewer outputs when it was not.
test.concurrent("plugin/entry point left without a module by onResolve fails the build", async () => {
using dir = tempDir("plugin-entry-point-without-module", {
"package.json": JSON.stringify({ name: "app", browser: { "./disabled.js": false } }),
"entry.js": `console.log("entry");`,
"other.js": `console.log("other");`,
"disabled.js": `console.log("disabled");`,
"build.mjs": `
const declined = [];
const plugins = [
{
name: "externalize-entry",
setup(build) {
build.onResolve({ filter: /entry\\.js$/ }, args => ({ path: args.path, external: true }));
},
},
{
name: "decline-disabled",
setup(build) {
build.onResolve({ filter: /disabled\\.js$/ }, args => {
declined.push(args.path);
});
},
},
];
const results = {};
for (const [name, entrypoints] of Object.entries({
external: ["./entry.js"],
externalNextToLiveEntryPoint: ["./entry.js", "./other.js"],
declinedThenDisabledByBrowserField: ["./disabled.js"],
})) {
const result = await Bun.build({ entrypoints, target: "browser", plugins, throw: false });
results[name] = {
success: result.success,
logs: result.logs.map(log => log.message),
outputs: result.outputs.map(output => output.path),
};
}
results.declined = declined;
console.log(JSON.stringify(results));
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "build.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(JSON.parse(stdout)).toEqual({
external: {
success: false,
logs: ['The entry point "./entry.js" cannot be marked as external'],
outputs: [],
},
externalNextToLiveEntryPoint: {
success: false,
logs: ['The entry point "./entry.js" cannot be marked as external'],
outputs: [],
},
declinedThenDisabledByBrowserField: {
success: false,
logs: ['"./disabled.js" is disabled due to "browser" field in package.json (entry point)'],
outputs: [],
},
declined: ["./disabled.js"],
});
expect(exitCode).toBe(0);
});
});