fix(npm): align Deno importing Node cjs with Node esm importing cjs (d…

…enoland#15879)
dplewis · Sep 12, 2022 · a3a4760 · a3a4760
1 parent 8f0a677
commit a3a4760
Show file tree

Hide file tree

Showing 5 changed files with 12 additions and 79 deletions.
diff --git a/cli/module_loader.rs b/cli/module_loader.rs
@@ -4,7 +4,6 @@ use crate::emit::emit_parsed_source;
 use crate::emit::TsTypeLib;
 use crate::graph_util::ModuleEntry;
 use crate::node;
-use crate::node::CjsToEsmTranslateKind;
 use crate::npm::NpmPackageResolver;
 use crate::proc_state::ProcState;
 use crate::text_encoding::code_without_source_map;
@@ -24,7 +23,6 @@ use deno_core::ModuleType;
 use deno_core::OpState;
 use deno_core::SourceMapGetter;
 use deno_runtime::permissions::Permissions;
-use std::borrow::Cow;
 use std::cell::RefCell;
 use std::pin::Pin;
 use std::rc::Rc;
@@ -136,28 +134,7 @@ impl CliModuleLoader {
  maybe_referrer: Option<ModuleSpecifier>,
  ) -> Result<ModuleSource, AnyError> {
  let code_source = if self.ps.npm_resolver.in_npm_package(specifier) {
- let is_cjs = self.ps.cjs_resolutions.lock().contains(specifier);
- let (maybe_translate_kind, load_specifier) = if is_cjs {
- let path = specifier.path();
- let mut specifier = specifier.clone();
- if let Some(new_path) = path.strip_suffix(node::CJS_TO_ESM_NODE_SUFFIX)
- {
- specifier.set_path(new_path);
- (Some(CjsToEsmTranslateKind::Node), Cow::Owned(specifier))
- } else if let Some(new_path) =
- path.strip_suffix(node::CJS_TO_ESM_DENO_SUFFIX)
- {
- specifier.set_path(new_path);
- (Some(CjsToEsmTranslateKind::Deno), Cow::Owned(specifier))
- } else {
- // all cjs code that goes through the loader should have been given a suffix
- unreachable!("Unknown cjs specifier: {}", specifier);
- }
- } else {
- (None, Cow::Borrowed(specifier))
- };
-
- let file_path = load_specifier.to_file_path().unwrap();
+ let file_path = specifier.to_file_path().unwrap();
  let code = std::fs::read_to_string(&file_path).with_context(|| {
  let mut msg = "Unable to load ".to_string();
  msg.push_str(&*file_path.to_string_lossy());
@@ -168,19 +145,18 @@ impl CliModuleLoader {
  msg
  })?;
 
- let code = if let Some(translate_kind) = maybe_translate_kind {
+ let code = if self.ps.cjs_resolutions.lock().contains(specifier) {
  // translate cjs to esm if it's cjs and inject node globals
  node::translate_cjs_to_esm(
  &self.ps.file_fetcher,
- &load_specifier,
+ specifier,
  code,
  MediaType::Cjs,
  &self.ps.npm_resolver,
- translate_kind,
  )?
  } else {
  // only inject node globals for esm
- node::esm_code_with_node_globals(&load_specifier, code)?
+ node::esm_code_with_node_globals(specifier, code)?
  };
  ModuleCodeSource {
  code,

diff --git a/cli/node/mod.rs b/cli/node/mod.rs
@@ -255,11 +255,6 @@ static NODE_COMPAT_URL: Lazy<Url> = Lazy::new(|| {
 pub static MODULE_ALL_URL: Lazy<Url> =
  Lazy::new(|| NODE_COMPAT_URL.join("node/module_all.ts").unwrap());
 
-/// Suffix used for the CJS to ESM translation for Node code
-pub const CJS_TO_ESM_NODE_SUFFIX: &str = ".node_cjs_to_esm.mjs";
-/// Suffix used for the CJS to ESM translation for Deno code
-pub const CJS_TO_ESM_DENO_SUFFIX: &str = ".deno_cjs_to_esm.mjs";
-
 fn find_builtin_node_module(specifier: &str) -> Option<&NodeModulePolyfill> {
  SUPPORTED_MODULES.iter().find(|m| m.name == specifier)
 }
@@ -417,15 +412,7 @@ pub fn node_resolve(
  None => return Ok(None),
  };
 
- let resolve_response = match url_to_node_resolution(url, npm_resolver)? {
- NodeResolution::CommonJs(mut url) => {
- // Use a suffix to say this cjs specifier should be translated from
- // cjs to esm for consumption by Node ESM code
- url.set_path(&format!("{}{}", url.path(), CJS_TO_ESM_NODE_SUFFIX));
- NodeResolution::CommonJs(url)
- }
- val => val,
- };
+ let resolve_response = url_to_node_resolution(url, npm_resolver)?;
  // TODO(bartlomieju): skipped checking errors for commonJS resolution and
  // "preserveSymlinksMain"/"preserveSymlinks" options.
  Ok(Some(resolve_response))
@@ -453,15 +440,7 @@ pub fn node_resolve_npm_reference(
  })?;
 
  let url = ModuleSpecifier::from_file_path(resolved_path).unwrap();
- let resolve_response = match url_to_node_resolution(url, npm_resolver)? {
- NodeResolution::CommonJs(mut url) => {
- // Use a suffix to say this cjs specifier should be translated from
- // cjs to esm for consumption by Deno ESM code
- url.set_path(&format!("{}{}", url.path(), CJS_TO_ESM_DENO_SUFFIX));
- NodeResolution::CommonJs(url)
- }
- val => val,
- };
+ let resolve_response = url_to_node_resolution(url, npm_resolver)?;
  // TODO(bartlomieju): skipped checking errors for commonJS resolution and
  // "preserveSymlinksMain"/"preserveSymlinks" options.
  Ok(Some(resolve_response))
@@ -722,12 +701,6 @@ fn add_export(
  }
 }
 
-#[derive(Clone, Copy, PartialEq, Eq)]
-pub enum CjsToEsmTranslateKind {
- Node,
- Deno,
-}
-
 /// Translates given CJS module into ESM. This function will perform static
 /// analysis on the file to find defined exports and reexports.
 ///
@@ -740,7 +713,6 @@ pub fn translate_cjs_to_esm(
  code: String,
  media_type: MediaType,
  npm_resolver: &GlobalNpmPackageResolver,
- translate_kind: CjsToEsmTranslateKind,
 ) -> Result<String, AnyError> {
  fn perform_cjs_analysis(
  specifier: &str,
@@ -767,11 +739,6 @@ pub fn translate_cjs_to_esm(
 
  let analysis = perform_cjs_analysis(specifier.as_str(), media_type, code)?;
 
- let root_exports = analysis
- .exports
- .iter()
- .map(|s| s.as_str())
- .collect::<HashSet<_>>();
  let mut all_exports = analysis
  .exports
  .iter()
@@ -837,16 +804,8 @@ pub fn translate_cjs_to_esm(
  .replace('\"', "\\\"")
  ));
 
- let mut had_default = false;
  for export in &all_exports {
- if export.as_str() == "default" {
- if translate_kind == CjsToEsmTranslateKind::Deno
- && root_exports.contains("__esModule")
- {
- source.push(format!("export default mod[\"{}\"];", export));
- had_default = true;
- }
- } else {
+ if export.as_str() != "default" {
  add_export(
  &mut source,
  export,
@@ -856,9 +815,7 @@ pub fn translate_cjs_to_esm(
  }
  }
 
- if !had_default {
- source.push("export default mod;".to_string());
- }
+ source.push("export default mod;".to_string());
 
  let translated_source = source.join("\n");
  Ok(translated_source)

diff --git a/cli/tests/testdata/npm/cjs_reexport_collision/main.ts b/cli/tests/testdata/npm/cjs_reexport_collision/main.ts
@@ -1,2 +1,2 @@
 import ReExportCollision from "npm:@denotest/cjs-reexport-collision";
-ReExportCollision.sayHello();
+ReExportCollision.default.sayHello();
diff --git a/cli/tests/testdata/npm/esm_import_cjs_default/main.js b/cli/tests/testdata/npm/esm_import_cjs_default/main.js
@@ -17,6 +17,6 @@ console.log(esmDefault);
 console.log(esmNamespace);
 console.log("===========================");
 
-console.log(cjsDefault());
+console.log(cjsDefault.default());
 console.log(esmDefault());
 console.log(MyCjsClass.someStaticMethod());
diff --git a/cli/tests/testdata/npm/esm_import_cjs_default/main.out b/cli/tests/testdata/npm/esm_import_cjs_default/main.out
@@ -17,11 +17,11 @@ Module {
 static method
 Deno esm importing node cjs
 ===========================
-[Function]
+{ default: [Function], named: [Function], MyClass: [Function: MyClass] }
 Module {
  MyClass: [Function: MyClass],
  __esModule: true,
- default: [Function],
+ default: { default: [Function], named: [Function], MyClass: [Function: MyClass] },
  named: [Function]
 }
 ===========================