From 9abc722cc3a998b4f73103c4394b53cccdb5c83b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 21 Mar 2024 11:35:51 -0700 Subject: [PATCH] feat(node): load ES modules defined as CJS (#22945) Changes the behaviour in Deno to just always load ES modules in npm packages even if they're defined as CJS. Closes #22818 --- cli/cache/node.rs | 19 +++--- cli/node.rs | 58 +++++++++++++------ cli/resolver.rs | 2 +- ext/node/analyze.rs | 31 +++++++++- ext/node/package_json.rs | 8 +-- .../__test__.jsonc | 10 ++++ .../detect_es_module_defined_as_cjs/deno.json | 5 ++ .../detect_es_module_defined_as_cjs/main.out | 1 + .../detect_es_module_defined_as_cjs/main.ts | 3 + .../node_modules/package/index.js | 3 + .../node_modules/package/package.json | 3 + .../package.json | 0 12 files changed, 107 insertions(+), 36 deletions(-) create mode 100644 tests/specs/node/detect_es_module_defined_as_cjs/__test__.jsonc create mode 100644 tests/specs/node/detect_es_module_defined_as_cjs/deno.json create mode 100644 tests/specs/node/detect_es_module_defined_as_cjs/main.out create mode 100644 tests/specs/node/detect_es_module_defined_as_cjs/main.ts create mode 100644 tests/specs/node/detect_es_module_defined_as_cjs/node_modules/package/index.js create mode 100644 tests/specs/node/detect_es_module_defined_as_cjs/node_modules/package/package.json create mode 100644 tests/specs/node/detect_es_module_defined_as_cjs/package.json diff --git a/cli/cache/node.rs b/cli/cache/node.rs index c9286c3822..29658bd90d 100644 --- a/cli/cache/node.rs +++ b/cli/cache/node.rs @@ -1,10 +1,11 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -use deno_ast::CjsAnalysis; use deno_core::error::AnyError; use deno_core::serde_json; use deno_runtime::deno_webstorage::rusqlite::params; +use crate::node::CliCjsAnalysis; + use super::cache_db::CacheDB; use super::cache_db::CacheDBConfiguration; use super::cache_db::CacheFailure; @@ -59,7 +60,7 @@ impl NodeAnalysisCache { &self, specifier: &str, expected_source_hash: &str, - ) -> Option { + ) -> Option { Self::ensure_ok( self.inner.get_cjs_analysis(specifier, expected_source_hash), ) @@ -69,7 +70,7 @@ impl NodeAnalysisCache { &self, specifier: &str, source_hash: &str, - cjs_analysis: &CjsAnalysis, + cjs_analysis: &CliCjsAnalysis, ) { Self::ensure_ok(self.inner.set_cjs_analysis( specifier, @@ -93,7 +94,7 @@ impl NodeAnalysisCacheInner { &self, specifier: &str, expected_source_hash: &str, - ) -> Result, AnyError> { + ) -> Result, AnyError> { let query = " SELECT data @@ -118,7 +119,7 @@ impl NodeAnalysisCacheInner { &self, specifier: &str, source_hash: &str, - cjs_analysis: &CjsAnalysis, + cjs_analysis: &CliCjsAnalysis, ) -> Result<(), AnyError> { let sql = " INSERT OR REPLACE INTO @@ -147,7 +148,7 @@ mod test { let cache = NodeAnalysisCacheInner::new(conn); assert!(cache.get_cjs_analysis("file.js", "2").unwrap().is_none()); - let cjs_analysis = CjsAnalysis { + let cjs_analysis = CliCjsAnalysis::Cjs { exports: vec!["export1".to_string()], reexports: vec!["re-export1".to_string()], }; @@ -157,8 +158,7 @@ mod test { assert!(cache.get_cjs_analysis("file.js", "3").unwrap().is_none()); // different hash let actual_cjs_analysis = cache.get_cjs_analysis("file.js", "2").unwrap().unwrap(); - assert_eq!(actual_cjs_analysis.exports, cjs_analysis.exports); - assert_eq!(actual_cjs_analysis.reexports, cjs_analysis.reexports); + assert_eq!(actual_cjs_analysis, cjs_analysis); // adding when already exists should not cause issue cache @@ -170,8 +170,7 @@ mod test { let cache = NodeAnalysisCacheInner::new(conn); let actual_analysis = cache.get_cjs_analysis("file.js", "2").unwrap().unwrap(); - assert_eq!(actual_analysis.exports, cjs_analysis.exports); - assert_eq!(actual_analysis.reexports, cjs_analysis.reexports); + assert_eq!(actual_analysis, cjs_analysis); // now changing the cli version should clear it let conn = cache.conn.recreate_with_version("2.0.0"); diff --git a/cli/node.rs b/cli/node.rs index cbe0aaaf1c..5f0ecc6533 100644 --- a/cli/node.rs +++ b/cli/node.rs @@ -1,15 +1,15 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -use std::borrow::Cow; - -use deno_ast::CjsAnalysis; use deno_ast::MediaType; use deno_ast::ModuleSpecifier; use deno_core::error::AnyError; use deno_runtime::deno_fs; use deno_runtime::deno_node::analyze::CjsAnalysis as ExtNodeCjsAnalysis; +use deno_runtime::deno_node::analyze::CjsAnalysisExports; use deno_runtime::deno_node::analyze::CjsCodeAnalyzer; use deno_runtime::deno_node::analyze::NodeCodeTranslator; +use serde::Deserialize; +use serde::Serialize; use crate::cache::NodeAnalysisCache; use crate::util::fs::canonicalize_path_maybe_not_exists; @@ -35,6 +35,17 @@ pub fn resolve_specifier_into_node_modules( .unwrap_or_else(|| specifier.clone()) } +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum CliCjsAnalysis { + /// The module was found to be an ES module. + Esm, + /// The module was CJS. + Cjs { + exports: Vec, + reexports: Vec, + }, +} + pub struct CliCjsCodeAnalyzer { cache: NodeAnalysisCache, fs: deno_fs::FileSystemRc, @@ -49,7 +60,7 @@ impl CliCjsCodeAnalyzer { &self, specifier: &ModuleSpecifier, source: &str, - ) -> Result { + ) -> Result { let source_hash = NodeAnalysisCache::compute_source_hash(source); if let Some(analysis) = self .cache @@ -60,13 +71,13 @@ impl CliCjsCodeAnalyzer { let media_type = MediaType::from_specifier(specifier); if media_type == MediaType::Json { - return Ok(CjsAnalysis { + return Ok(CliCjsAnalysis::Cjs { exports: vec![], reexports: vec![], }); } - let parsed_source = deno_ast::parse_script(deno_ast::ParseParams { + let parsed_source = deno_ast::parse_program(deno_ast::ParseParams { specifier: specifier.clone(), text_info: deno_ast::SourceTextInfo::new(source.into()), media_type, @@ -74,7 +85,15 @@ impl CliCjsCodeAnalyzer { scope_analysis: false, maybe_syntax: None, })?; - let analysis = parsed_source.analyze_cjs(); + let analysis = if parsed_source.is_script() { + let analysis = parsed_source.analyze_cjs(); + CliCjsAnalysis::Cjs { + exports: analysis.exports, + reexports: analysis.reexports, + } + } else { + CliCjsAnalysis::Esm + }; self .cache .set_cjs_analysis(specifier.as_str(), &source_hash, &analysis); @@ -87,20 +106,23 @@ impl CjsCodeAnalyzer for CliCjsCodeAnalyzer { fn analyze_cjs( &self, specifier: &ModuleSpecifier, - source: Option<&str>, + source: Option, ) -> Result { let source = match source { - Some(source) => Cow::Borrowed(source), - None => Cow::Owned( - self - .fs - .read_text_file_sync(&specifier.to_file_path().unwrap())?, - ), + Some(source) => source, + None => self + .fs + .read_text_file_sync(&specifier.to_file_path().unwrap())?, }; let analysis = self.inner_cjs_analysis(specifier, &source)?; - Ok(ExtNodeCjsAnalysis { - exports: analysis.exports, - reexports: analysis.reexports, - }) + match analysis { + CliCjsAnalysis::Esm => Ok(ExtNodeCjsAnalysis::Esm(source)), + CliCjsAnalysis::Cjs { exports, reexports } => { + Ok(ExtNodeCjsAnalysis::Cjs(CjsAnalysisExports { + exports, + reexports, + })) + } + } } } diff --git a/cli/resolver.rs b/cli/resolver.rs index 495b3f9b5e..6594bf2d4d 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -334,7 +334,7 @@ impl NpmModuleLoader { // translate cjs to esm if it's cjs and inject node globals self.node_code_translator.translate_cjs_to_esm( specifier, - Some(code.as_str()), + Some(code), permissions, )? } else { diff --git a/ext/node/analyze.rs b/ext/node/analyze.rs index 2a0324ccf7..0a02266254 100644 --- a/ext/node/analyze.rs +++ b/ext/node/analyze.rs @@ -5,6 +5,7 @@ use std::collections::VecDeque; use std::path::Path; use std::path::PathBuf; +use deno_core::anyhow; use deno_core::anyhow::Context; use deno_core::ModuleSpecifier; use once_cell::sync::Lazy; @@ -21,7 +22,15 @@ use crate::PackageJson; use crate::PathClean; #[derive(Debug, Clone)] -pub struct CjsAnalysis { +pub enum CjsAnalysis { + /// File was found to be an ES module and the translator should + /// load the code as ESM. + Esm(String), + Cjs(CjsAnalysisExports), +} + +#[derive(Debug, Clone)] +pub struct CjsAnalysisExports { pub exports: Vec, pub reexports: Vec, } @@ -38,7 +47,7 @@ pub trait CjsCodeAnalyzer { fn analyze_cjs( &self, specifier: &ModuleSpecifier, - maybe_source: Option<&str>, + maybe_source: Option, ) -> Result; } @@ -73,7 +82,7 @@ impl NodeCodeTranslator { pub fn translate_cjs_to_esm( &self, specifier: &ModuleSpecifier, - source: Option<&str>, + source: Option, permissions: &dyn NodePermissions, ) -> Result { let mut temp_var_count = 0; @@ -81,6 +90,11 @@ impl NodeCodeTranslator { let analysis = self.cjs_code_analyzer.analyze_cjs(specifier, source)?; + let analysis = match analysis { + CjsAnalysis::Esm(source) => return Ok(source), + CjsAnalysis::Cjs(analysis) => analysis, + }; + let mut source = vec![ r#"import {createRequire as __internalCreateRequire} from "node:module"; const require = __internalCreateRequire(import.meta.url);"# @@ -127,6 +141,17 @@ impl NodeCodeTranslator { reexport, reexport_specifier, referrer ) })?; + let analysis = match analysis { + CjsAnalysis::Esm(_) => { + // todo(dsherret): support this once supporting requiring ES modules + return Err(anyhow::anyhow!( + "Cannot require ES module '{}' from '{}'", + reexport_specifier, + specifier + )); + } + CjsAnalysis::Cjs(analysis) => analysis, + }; for reexport in analysis.reexports { reexports_to_handle.push_back((reexport, reexport_specifier.clone())); diff --git a/ext/node/package_json.rs b/ext/node/package_json.rs index 7fcfb69176..9d6a491b53 100644 --- a/ext/node/package_json.rs +++ b/ext/node/package_json.rs @@ -93,10 +93,6 @@ impl PackageJson { ), }; - if source.trim().is_empty() { - return Ok(PackageJson::empty(path)); - } - let package_json = Self::load_from_string(path, source)?; CACHE.with(|cache| { cache @@ -110,6 +106,10 @@ impl PackageJson { path: PathBuf, source: String, ) -> Result { + if source.trim().is_empty() { + return Ok(PackageJson::empty(path)); + } + let package_json: Value = serde_json::from_str(&source).map_err(|err| { anyhow::anyhow!( "malformed package.json: {}\n at {}", diff --git a/tests/specs/node/detect_es_module_defined_as_cjs/__test__.jsonc b/tests/specs/node/detect_es_module_defined_as_cjs/__test__.jsonc new file mode 100644 index 0000000000..fcb9dc1436 --- /dev/null +++ b/tests/specs/node/detect_es_module_defined_as_cjs/__test__.jsonc @@ -0,0 +1,10 @@ +{ + "steps": [{ + "args": "run main.ts", + "output": "main.out" + }, { + // ensure it works with cache + "args": "run main.ts", + "output": "main.out" + }] +} diff --git a/tests/specs/node/detect_es_module_defined_as_cjs/deno.json b/tests/specs/node/detect_es_module_defined_as_cjs/deno.json new file mode 100644 index 0000000000..f6eb03cc93 --- /dev/null +++ b/tests/specs/node/detect_es_module_defined_as_cjs/deno.json @@ -0,0 +1,5 @@ +{ + "unstable": [ + "byonm" + ] +} diff --git a/tests/specs/node/detect_es_module_defined_as_cjs/main.out b/tests/specs/node/detect_es_module_defined_as_cjs/main.out new file mode 100644 index 0000000000..00750edc07 --- /dev/null +++ b/tests/specs/node/detect_es_module_defined_as_cjs/main.out @@ -0,0 +1 @@ +3 diff --git a/tests/specs/node/detect_es_module_defined_as_cjs/main.ts b/tests/specs/node/detect_es_module_defined_as_cjs/main.ts new file mode 100644 index 0000000000..a4be7b4c4f --- /dev/null +++ b/tests/specs/node/detect_es_module_defined_as_cjs/main.ts @@ -0,0 +1,3 @@ +import { add } from "package"; + +console.log(add(1, 2)); diff --git a/tests/specs/node/detect_es_module_defined_as_cjs/node_modules/package/index.js b/tests/specs/node/detect_es_module_defined_as_cjs/node_modules/package/index.js new file mode 100644 index 0000000000..7d658310b0 --- /dev/null +++ b/tests/specs/node/detect_es_module_defined_as_cjs/node_modules/package/index.js @@ -0,0 +1,3 @@ +export function add(a, b) { + return a + b; +} diff --git a/tests/specs/node/detect_es_module_defined_as_cjs/node_modules/package/package.json b/tests/specs/node/detect_es_module_defined_as_cjs/node_modules/package/package.json new file mode 100644 index 0000000000..920dda0800 --- /dev/null +++ b/tests/specs/node/detect_es_module_defined_as_cjs/node_modules/package/package.json @@ -0,0 +1,3 @@ +{ + "name": "package" +} diff --git a/tests/specs/node/detect_es_module_defined_as_cjs/package.json b/tests/specs/node/detect_es_module_defined_as_cjs/package.json new file mode 100644 index 0000000000..e69de29bb2