diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index a2a31fd5e974..1db3cef3ee63 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -2357,39 +2357,15 @@ pub mod bv2_impl { [import_record.importer_source_index as usize], ) .expect("oom"); - - // Turn this into an invalid AST, so that incremental mode skips it when printing. - // SAFETY: truncating to len 0 never exposes uninitialized elements. - unsafe { - self.graph.ast.items_parts_mut() - [import_record.importer_source_index as usize] - .set_len((0) as usize) - }; } } - let handles_import_errors; - // reshaped for borrowck — `log_for_resolution_failures` borrows - // `&mut self`; the returned log is backed by either a DevServer-owned slot or - // `*self.transpiler.log` (both raw-pointer-derived), so detach the lifetime - // so `self.graph.*` / `self.transpiler.*` reads below type-check. - // SAFETY: log lives in DevServer / transpiler, disjoint from `self.graph`. - let log: &mut bun_ast::Log = unsafe { - bun_ptr::detach_lifetime_mut(self.log_for_resolution_failures( - &import_record.source_file, - target.bake_graph(), - )) - }; - - { + let handles_import_errors = { let record: &mut ImportRecord = &mut self.graph.ast.items_import_records_mut() [import_record.importer_source_index as usize] .as_mut_slice() [import_record.import_record_index as usize]; - handles_import_errors = record - .flags - .contains(bun_ast::ImportRecordFlags::HANDLES_IMPORT_ERRORS); // Disable failing packages from being printed. // This may cause broken code to write. @@ -2399,61 +2375,88 @@ pub mod bv2_impl { record .flags .insert(bun_ast::ImportRecordFlags::WAS_UNRESOLVED); - } - let source: Option<&bun_ast::Source> = Some( - &self.graph.input_files.items_source() - [import_record.importer_source_index as usize], - ); + record + .flags + .contains(bun_ast::ImportRecordFlags::HANDLES_IMPORT_ERRORS) + }; - if err == _resolver::Error::ModuleNotFound { + if err == _resolver::Error::ModuleNotFound + && !handles_import_errors + && !self.transpiler.options.ignore_module_resolution_errors + { + if self.dev_server.is_some() { + // Turn this into an invalid AST, so that incremental mode skips it + // when printing. Only valid because a failure is logged for the + // importer below: a file with no parts and no failure (e.g. a + // `try { require() }` of a missing file) would otherwise vanish + // from the incremental graph instead of being bundled. + // SAFETY: truncating to len 0 never exposes uninitialized elements. + unsafe { + self.graph.ast.items_parts_mut() + [import_record.importer_source_index as usize] + .set_len(0) + }; + } + + // reshaped for borrowck — `log_for_resolution_failures` borrows + // `&mut self`; the returned log is backed by either a DevServer-owned slot or + // `*self.transpiler.log` (both raw-pointer-derived), so detach the lifetime + // so the `self.graph.*` read below type-checks. + // SAFETY: log lives in DevServer / transpiler, disjoint from `self.graph`. + let log: &mut bun_ast::Log = unsafe { + bun_ptr::detach_lifetime_mut(self.log_for_resolution_failures( + &import_record.source_file, + target.bake_graph(), + )) + }; + let source: Option<&bun_ast::Source> = Some( + &self.graph.input_files.items_source() + [import_record.importer_source_index as usize], + ); let add_error = bun_ast::Log::add_resolve_error_with_text_dupe; let path_to_use = &import_record.specifier; - if !handles_import_errors - && !self.transpiler.options.ignore_module_resolution_errors - { - if is_package_path(&import_record.specifier) { - if target == Target::Browser - && options::is_node_builtin(path_to_use) - { - add_error( - log, - source, - import_record.range, - format_args!( - "Browser build cannot {} Node.js module: \"{}\". To use Node.js builtins, set target to 'node' or 'bun'", - bstr::BStr::new(import_record.kind.error_label()), - bstr::BStr::new(path_to_use) - ), - path_to_use, - import_record.kind, - ); - } else { - add_error( - log, - source, - import_record.range, - format_args!( - "Could not resolve: \"{}\". Maybe you need to \"bun install\"?", - bstr::BStr::new(path_to_use) - ), - path_to_use, - import_record.kind, - ); - } + if is_package_path(&import_record.specifier) { + if target == Target::Browser + && options::is_node_builtin(path_to_use) + { + add_error( + log, + source, + import_record.range, + format_args!( + "Browser build cannot {} Node.js module: \"{}\". To use Node.js builtins, set target to 'node' or 'bun'", + bstr::BStr::new(import_record.kind.error_label()), + bstr::BStr::new(path_to_use) + ), + path_to_use, + import_record.kind, + ); } else { add_error( log, source, import_record.range, format_args!( - "Could not resolve: \"{}\"", + "Could not resolve: \"{}\". Maybe you need to \"bun install\"?", bstr::BStr::new(path_to_use) ), path_to_use, import_record.kind, ); } + } else { + add_error( + log, + source, + import_record.range, + format_args!( + "Could not resolve: \"{}\"", + bstr::BStr::new(path_to_use) + ), + path_to_use, + import_record.kind, + ); } } // assume other errors are already in the log diff --git a/test/bake/dev/plugins.test.ts b/test/bake/dev/plugins.test.ts index fa1641f878af..28ce82fc6a7b 100644 --- a/test/bake/dev/plugins.test.ts +++ b/test/bake/dev/plugins.test.ts @@ -1,5 +1,5 @@ // Plugin tests concern plugins in development mode. -import { devTest, minimalFramework } from "../bake-harness"; +import { devTest, emptyHtmlFile, minimalFramework } from "../bake-harness"; // Note: more in depth testing of plugins is done in test/bundler/bundler_plugin.test.ts devTest("onResolve", { @@ -147,3 +147,67 @@ devTest("onResolve + onLoad virtual file", { // await dev.fetch("/").expect('value: 2'); // }, // }); + +// When an onResolve callback returns nothing, the specifier falls through to +// the builtin resolver (BundleV2::run_resolver). A resolution failure on that +// path that the importer handles itself (require inside try/catch) is not an +// error, so the importer must still be bundled. +const onResolveFallThroughPlugin = { + "bunfig.toml": ` + [serve.static] + plugins = ["./plugin.ts"] + `, + "plugin.ts": ` + export default { + name: "fall-through", + setup(build) { + build.onResolve({ filter: /optional-dep|missing/ }, () => undefined); + }, + }; + `, +}; +devTest("onResolve fall-through keeps a module whose missing require is in try/catch", { + files: { + ...onResolveFallThroughPlugin, + "index.html": emptyHtmlFile({ scripts: ["index.ts"] }), + "index.ts": ` + let value = "fallback"; + try { + value = require("./optional-dep").value; + } catch {} + console.log("v1 " + value); + import.meta.hot.accept(); + `, + }, + async test(dev) { + await using c = await dev.client("/"); + await c.expectMessage("v1 fallback"); + + // The module is in the incremental graph, so editing it is a hot update. + await dev.patch("index.ts", { find: "v1", replace: "v2" }); + await c.expectMessage("v2 fallback"); + + // The failed resolution is still tracked: creating the file re-bundles the importer. + await dev.write("optional-dep.ts", `export const value = "dep";`); + await c.expectMessage("v2 dep"); + }, +}); +devTest("onResolve fall-through still reports an unresolvable import", { + files: { + ...onResolveFallThroughPlugin, + "index.html": emptyHtmlFile({ scripts: ["index.ts"] }), + "index.ts": ` + import { value } from "./missing"; + console.log(value); + `, + }, + async test(dev) { + await using c = await dev.client("/", { + errors: [`index.ts:1:23: error: Could not resolve: "./missing"`], + }); + await c.expectReload(async () => { + await dev.write("missing.ts", `export const value = "found";`); + }); + await c.expectMessage("found"); + }, +}); diff --git a/test/bundler/bundler_plugin.test.ts b/test/bundler/bundler_plugin.test.ts index 0b9ff951d5dd..f23703c0277e 100644 --- a/test/bundler/bundler_plugin.test.ts +++ b/test/bundler/bundler_plugin.test.ts @@ -466,6 +466,40 @@ describe("bundler", () => { }, }; }); + // An onResolve callback that returns nothing falls through to the builtin + // resolver (BundleV2::run_resolver), which has to treat a file it cannot find + // the same way the synchronous resolver does. + itBundled("plugin/ResolveFallThroughMissingRequireInTryCatch", { + files: { + "index.ts": /* ts */ ` + let value = "fallback"; + try { + value = require("./optional-dep").value; + } catch {} + console.log(value); + `, + }, + plugins(builder) { + builder.onResolve({ filter: /optional-dep/ }, () => undefined); + }, + run: { + stdout: "fallback", + }, + }); + itBundled("plugin/ResolveFallThroughMissingImport", { + files: { + "index.ts": /* ts */ ` + import { value } from "./missing"; + console.log(value); + `, + }, + plugins(builder) { + builder.onResolve({ filter: /missing/ }, () => undefined); + }, + bundleErrors: { + "/index.ts": [`Could not resolve: "./missing"`], + }, + }); itBundled("plugin/ResolveOnceWhenSameFile", ({ root }) => { let onResolveCount = 0; return {