From f1ba26661346a83b6e7fe5e7ffeed4553a9571ae Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Thu, 19 Sep 2024 18:37:36 -0700 Subject: [PATCH] fix(node): Don't error out if we fail to statically analyze CJS re-export (#25748) Fixes rsbuild running in deno. You can look at the test to see what was failing, the gist is that we were trying to statically analyze the re-exports of a CJS script, and if we couldn't find the source for the re-exported file we would fail. Instead, we should just treat these as if they were too dynamic to analyze, and let it fail (or succeed) at runtime. This aligns with node's behavior. --- cli/node.rs | 21 ++++- ext/node_resolver/analyze.rs | 80 ++++++++++++------- .../__test__.jsonc | 9 +++ .../run/cjs_reexport_non_analyzable/deno.json | 3 + .../run/cjs_reexport_non_analyzable/main.ts | 3 + .../node_modules/foo.cjs | 5 ++ 6 files changed, 88 insertions(+), 33 deletions(-) create mode 100644 tests/specs/run/cjs_reexport_non_analyzable/__test__.jsonc create mode 100644 tests/specs/run/cjs_reexport_non_analyzable/deno.json create mode 100644 tests/specs/run/cjs_reexport_non_analyzable/main.ts create mode 100644 tests/specs/run/cjs_reexport_non_analyzable/node_modules/foo.cjs diff --git a/cli/node.rs b/cli/node.rs index 0fd18e2991..766da1c014 100644 --- a/cli/node.rs +++ b/cli/node.rs @@ -125,10 +125,23 @@ impl CjsCodeAnalyzer for CliCjsCodeAnalyzer { let source = match source { Some(source) => source, None => { - self - .fs - .read_text_file_lossy_async(specifier.to_file_path().unwrap(), None) - .await? + if let Ok(path) = specifier.to_file_path() { + if let Ok(source_from_file) = + self.fs.read_text_file_lossy_async(path, None).await + { + source_from_file + } else { + return Ok(ExtNodeCjsAnalysis::Cjs(CjsAnalysisExports { + exports: vec![], + reexports: vec![], + })); + } + } else { + return Ok(ExtNodeCjsAnalysis::Cjs(CjsAnalysisExports { + exports: vec![], + reexports: vec![], + })); + } } }; let analysis = self.inner_cjs_analysis(specifier, &source).await?; diff --git a/ext/node_resolver/analyze.rs b/ext/node_resolver/analyze.rs index 8231cdc8a4..aea33e29ad 100644 --- a/ext/node_resolver/analyze.rs +++ b/ext/node_resolver/analyze.rs @@ -200,7 +200,8 @@ impl NodeResolutionMode::Execution, ); let reexport_specifier = match result { - Ok(specifier) => specifier, + Ok(Some(specifier)) => specifier, + Ok(None) => continue, Err(err) => { errors.push(err); continue; @@ -291,7 +292,7 @@ impl referrer: &Url, conditions: &[&str], mode: NodeResolutionMode, - ) -> Result { + ) -> Result, AnyError> { if specifier.starts_with('/') { todo!(); } @@ -299,9 +300,12 @@ impl let referrer_path = referrer.to_file_path().unwrap(); if specifier.starts_with("./") || specifier.starts_with("../") { if let Some(parent) = referrer_path.parent() { - return self - .file_extension_probe(parent.join(specifier), &referrer_path) - .map(|p| to_file_specifier(&p)); + return Some( + self + .file_extension_probe(parent.join(specifier), &referrer_path) + .map(|p| to_file_specifier(&p)), + ) + .transpose(); } else { todo!(); } @@ -311,28 +315,41 @@ impl let (package_specifier, package_subpath) = parse_specifier(specifier).unwrap(); - let module_dir = self.npm_resolver.resolve_package_folder_from_package( - package_specifier.as_str(), - referrer, - )?; + let module_dir = match self + .npm_resolver + .resolve_package_folder_from_package(package_specifier.as_str(), referrer) + { + Err(err) + if matches!( + err.as_kind(), + crate::errors::PackageFolderResolveErrorKind::PackageNotFound(..) + ) => + { + return Ok(None); + } + other => other, + }?; let package_json_path = module_dir.join("package.json"); let maybe_package_json = load_pkg_json(self.env.pkg_json_fs(), &package_json_path)?; if let Some(package_json) = maybe_package_json { if let Some(exports) = &package_json.exports { - return self - .node_resolver - .package_exports_resolve( - &package_json_path, - &package_subpath, - exports, - Some(referrer), - NodeModuleKind::Esm, - conditions, - mode, - ) - .map_err(AnyError::from); + return Some( + self + .node_resolver + .package_exports_resolve( + &package_json_path, + &package_subpath, + exports, + Some(referrer), + NodeModuleKind::Esm, + conditions, + mode, + ) + .map_err(AnyError::from), + ) + .transpose(); } // old school @@ -345,19 +362,24 @@ impl load_pkg_json(self.env.pkg_json_fs(), &package_json_path)?; if let Some(package_json) = maybe_package_json { if let Some(main) = package_json.main(NodeModuleKind::Cjs) { - return Ok(to_file_specifier(&d.join(main).clean())); + return Ok(Some(to_file_specifier(&d.join(main).clean()))); } } - return Ok(to_file_specifier(&d.join("index.js").clean())); + return Ok(Some(to_file_specifier(&d.join("index.js").clean()))); } - return self - .file_extension_probe(d, &referrer_path) - .map(|p| to_file_specifier(&p)); + return Some( + self + .file_extension_probe(d, &referrer_path) + .map(|p| to_file_specifier(&p)), + ) + .transpose(); } else if let Some(main) = package_json.main(NodeModuleKind::Cjs) { - return Ok(to_file_specifier(&module_dir.join(main).clean())); + return Ok(Some(to_file_specifier(&module_dir.join(main).clean()))); } else { - return Ok(to_file_specifier(&module_dir.join("index.js").clean())); + return Ok(Some(to_file_specifier( + &module_dir.join("index.js").clean(), + ))); } } @@ -373,7 +395,7 @@ impl parent.join("node_modules").join(specifier) }; if let Ok(path) = self.file_extension_probe(path, &referrer_path) { - return Ok(to_file_specifier(&path)); + return Ok(Some(to_file_specifier(&path))); } last = parent; } diff --git a/tests/specs/run/cjs_reexport_non_analyzable/__test__.jsonc b/tests/specs/run/cjs_reexport_non_analyzable/__test__.jsonc new file mode 100644 index 0000000000..81f29b685d --- /dev/null +++ b/tests/specs/run/cjs_reexport_non_analyzable/__test__.jsonc @@ -0,0 +1,9 @@ +{ + "tempDir": true, + "steps": [ + { + "args": "run -A main.ts", + "output": "" + } + ] +} diff --git a/tests/specs/run/cjs_reexport_non_analyzable/deno.json b/tests/specs/run/cjs_reexport_non_analyzable/deno.json new file mode 100644 index 0000000000..fde86a1efb --- /dev/null +++ b/tests/specs/run/cjs_reexport_non_analyzable/deno.json @@ -0,0 +1,3 @@ +{ + "nodeModulesDir": "manual" +} diff --git a/tests/specs/run/cjs_reexport_non_analyzable/main.ts b/tests/specs/run/cjs_reexport_non_analyzable/main.ts new file mode 100644 index 0000000000..a9e32af4e1 --- /dev/null +++ b/tests/specs/run/cjs_reexport_non_analyzable/main.ts @@ -0,0 +1,3 @@ +import assert from "./node_modules/foo.cjs"; + +assert.equal(1 + 1, 2); diff --git a/tests/specs/run/cjs_reexport_non_analyzable/node_modules/foo.cjs b/tests/specs/run/cjs_reexport_non_analyzable/node_modules/foo.cjs new file mode 100644 index 0000000000..924d8a96a9 --- /dev/null +++ b/tests/specs/run/cjs_reexport_non_analyzable/node_modules/foo.cjs @@ -0,0 +1,5 @@ +try { + module.exports = require("nonexistent"); +} catch(_e) { + module.exports = require("assert"); +}