Skip to content

Commit

Permalink
fix: cjs resolution cases (#25739)
Browse files Browse the repository at this point in the history
Fixes cjs modules being loaded as esm.
  • Loading branch information
devsnek authored Sep 20, 2024
1 parent f1ba266 commit a01dce3
Show file tree
Hide file tree
Showing 11 changed files with 83 additions and 17 deletions.
8 changes: 6 additions & 2 deletions cli/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -572,8 +572,12 @@ impl CliFactory {
let caches = self.caches()?;
let node_analysis_cache =
NodeAnalysisCache::new(caches.node_analysis_db());
let cjs_esm_analyzer =
CliCjsCodeAnalyzer::new(node_analysis_cache, self.fs().clone());
let node_resolver = self.cli_node_resolver().await?.clone();
let cjs_esm_analyzer = CliCjsCodeAnalyzer::new(
node_analysis_cache,
self.fs().clone(),
node_resolver,
);

Ok(Arc::new(NodeCodeTranslator::new(
cjs_esm_analyzer,
Expand Down
39 changes: 36 additions & 3 deletions cli/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use serde::Serialize;

use crate::cache::CacheDBHash;
use crate::cache::NodeAnalysisCache;
use crate::resolver::CliNodeResolver;
use crate::util::fs::canonicalize_path_maybe_not_exists;

pub type CliNodeCodeTranslator =
Expand Down Expand Up @@ -54,11 +55,20 @@ pub enum CliCjsAnalysis {
pub struct CliCjsCodeAnalyzer {
cache: NodeAnalysisCache,
fs: deno_fs::FileSystemRc,
node_resolver: Arc<CliNodeResolver>,
}

impl CliCjsCodeAnalyzer {
pub fn new(cache: NodeAnalysisCache, fs: deno_fs::FileSystemRc) -> Self {
Self { cache, fs }
pub fn new(
cache: NodeAnalysisCache,
fs: deno_fs::FileSystemRc,
node_resolver: Arc<CliNodeResolver>,
) -> Self {
Self {
cache,
fs,
node_resolver,
}
}

async fn inner_cjs_analysis(
Expand All @@ -73,14 +83,30 @@ impl CliCjsCodeAnalyzer {
return Ok(analysis);
}

let media_type = MediaType::from_specifier(specifier);
let mut media_type = MediaType::from_specifier(specifier);
if media_type == MediaType::Json {
return Ok(CliCjsAnalysis::Cjs {
exports: vec![],
reexports: vec![],
});
}

if media_type == MediaType::JavaScript {
if let Some(package_json) =
self.node_resolver.get_closest_package_json(specifier)?
{
match package_json.typ.as_str() {
"commonjs" => {
media_type = MediaType::Cjs;
}
"module" => {
media_type = MediaType::Mjs;
}
_ => {}
}
}
}

let analysis = deno_core::unsync::spawn_blocking({
let specifier = specifier.clone();
let source: Arc<str> = source.into();
Expand All @@ -99,6 +125,13 @@ impl CliCjsCodeAnalyzer {
exports: analysis.exports,
reexports: analysis.reexports,
})
} else if media_type == MediaType::Cjs {
// FIXME: `deno_ast` should internally handle MediaType::Cjs implying that
// the result must never be Esm
Ok(CliCjsAnalysis::Cjs {
exports: vec![],
reexports: vec![],
})
} else {
Ok(CliCjsAnalysis::Esm)
}
Expand Down
19 changes: 11 additions & 8 deletions cli/standalone/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -580,8 +580,17 @@ pub async fn run(
let cjs_resolutions = Arc::new(CjsResolutionStore::default());
let cache_db = Caches::new(deno_dir_provider.clone());
let node_analysis_cache = NodeAnalysisCache::new(cache_db.node_analysis_db());
let cjs_esm_code_analyzer =
CliCjsCodeAnalyzer::new(node_analysis_cache, fs.clone());
let cli_node_resolver = Arc::new(CliNodeResolver::new(
cjs_resolutions.clone(),
fs.clone(),
node_resolver.clone(),
npm_resolver.clone(),
));
let cjs_esm_code_analyzer = CliCjsCodeAnalyzer::new(
node_analysis_cache,
fs.clone(),
cli_node_resolver.clone(),
);
let node_code_translator = Arc::new(NodeCodeTranslator::new(
cjs_esm_code_analyzer,
deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()),
Expand Down Expand Up @@ -637,12 +646,6 @@ pub async fn run(
metadata.workspace_resolver.pkg_json_resolution,
)
};
let cli_node_resolver = Arc::new(CliNodeResolver::new(
cjs_resolutions.clone(),
fs.clone(),
node_resolver.clone(),
npm_resolver.clone(),
));
let module_loader_factory = StandaloneModuleLoaderFactory {
shared: Arc::new(SharedModuleLoaderState {
eszip: WorkspaceEszip {
Expand Down
3 changes: 2 additions & 1 deletion cli/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,8 @@ impl CliMainWorkerFactory {
let is_main_cjs = matches!(node_resolution, NodeResolution::CommonJs(_));
(node_resolution.into_url(), is_main_cjs)
} else {
(main_module, false)
let is_cjs = main_module.path().ends_with(".cjs");
(main_module, is_cjs)
};

let ModuleLoaderAndSourceMapGetter { module_loader } =
Expand Down
7 changes: 5 additions & 2 deletions ext/node/polyfills/01_require.js
Original file line number Diff line number Diff line change
Expand Up @@ -1007,7 +1007,10 @@ Module.prototype._compile = function (content, filename, format) {
try {
compiledWrapper = wrapSafe(filename, content, this, format);
} catch (err) {
if (err instanceof SyntaxError && op_require_can_parse_as_esm(content)) {
if (
format !== "commonjs" && err instanceof SyntaxError &&
op_require_can_parse_as_esm(content)
) {
return loadESMFromCJS(this, filename, content);
}
throw err;
Expand Down Expand Up @@ -1067,7 +1070,7 @@ Module._extensions[".js"] = function (module, filename) {
let format;
if (StringPrototypeEndsWith(filename, ".js")) {
const pkg = op_require_read_closest_package_json(filename);
if (pkg?.typ === "module") {
if (pkg?.type === "module") {
format = "module";
} else if (pkg?.type === "commonjs") {
format = "commonjs";
Expand Down
1 change: 1 addition & 0 deletions tests/registry/npm/@denotest/type-commonjs/1.0.0/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export {};
5 changes: 5 additions & 0 deletions tests/registry/npm/@denotest/type-commonjs/1.0.0/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "@denotest/type-commonjs",
"version": "1.0.0",
"type": "commonjs"
}
5 changes: 5 additions & 0 deletions tests/specs/npm/require_type_commonjs/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"args": "run --allow-read --quiet main.ts",
"output": "main.out",
"exitCode": 1
}
10 changes: 10 additions & 0 deletions tests/specs/npm/require_type_commonjs/main.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: Uncaught (in promise) SyntaxError: Unexpected token 'export'
at Object.evalContext (ext:core/01_core.js:[WILDCARD])
at wrapSafe (node:module:[WILDCARD])
at Module._compile (node:module:[WILDCARD])
at Object.Module._extensions..js (node:module:[WILDCARD])
at Module.load (node:module:[WILDCARD])
at Function.Module._load (node:module:[WILDCARD])
at Module.require (node:module:[WILDCARD])
at require (node:module:[WILDCARD])
at file:///[WILDCARD]/@denotest/type-commonjs/1.0.0/index.js:3:13
1 change: 1 addition & 0 deletions tests/specs/npm/require_type_commonjs/main.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import "npm:@denotest/type-commonjs";
2 changes: 1 addition & 1 deletion tests/specs/run/require_esm/main.out
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[Module: null prototype] { sync_js: 1 }
[Module: null prototype] { sync_mjs: 1 }
error: Uncaught (in promise) Error: Top-level await is not allowed in synchronous evaluation
error: Uncaught Error: Top-level await is not allowed in synchronous evaluation
at loadESMFromCJS (node:module:[WILDCARD])
at Module._compile (node:module:[WILDCARD])
at Object.Module._extensions..js (node:module:[WILDCARD])
Expand Down

0 comments on commit a01dce3

Please sign in to comment.