Skip to content

Commit

Permalink
fix(node): Don't error out if we fail to statically analyze CJS re-ex…
Browse files Browse the repository at this point in the history
…port (#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.
  • Loading branch information
nathanwhit committed Sep 20, 2024
1 parent 5bcea1a commit f1ba266
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 33 deletions.
21 changes: 17 additions & 4 deletions cli/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?;
Expand Down
80 changes: 51 additions & 29 deletions ext/node_resolver/analyze.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer, TNodeResolverEnv: NodeResolverEnv>
NodeResolutionMode::Execution,
);
let reexport_specifier = match result {
Ok(specifier) => specifier,
Ok(Some(specifier)) => specifier,
Ok(None) => continue,
Err(err) => {
errors.push(err);
continue;
Expand Down Expand Up @@ -291,17 +292,20 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer, TNodeResolverEnv: NodeResolverEnv>
referrer: &Url,
conditions: &[&str],
mode: NodeResolutionMode,
) -> Result<Url, AnyError> {
) -> Result<Option<Url>, AnyError> {
if specifier.starts_with('/') {
todo!();
}

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!();
}
Expand All @@ -311,28 +315,41 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer, TNodeResolverEnv: NodeResolverEnv>
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
Expand All @@ -345,19 +362,24 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer, TNodeResolverEnv: NodeResolverEnv>
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(),
)));
}
}

Expand All @@ -373,7 +395,7 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer, TNodeResolverEnv: NodeResolverEnv>
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;
}
Expand Down
9 changes: 9 additions & 0 deletions tests/specs/run/cjs_reexport_non_analyzable/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"tempDir": true,
"steps": [
{
"args": "run -A main.ts",
"output": ""
}
]
}
3 changes: 3 additions & 0 deletions tests/specs/run/cjs_reexport_non_analyzable/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"nodeModulesDir": "manual"
}
3 changes: 3 additions & 0 deletions tests/specs/run/cjs_reexport_non_analyzable/main.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import assert from "./node_modules/foo.cjs";

assert.equal(1 + 1, 2);

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit f1ba266

Please sign in to comment.