fix(npm): align Node esm code importing cjs with Node (#15838)

cli/module_loader.rs

@@ -4,6 +4,7 @@ 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;
@@ -23,6 +24,7 @@ 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;
@@ -134,30 +136,51 @@ impl CliModuleLoader {
     maybe_referrer: Option<ModuleSpecifier>,
   ) -> Result<ModuleSource, AnyError> {
     let code_source = if self.ps.npm_resolver.in_npm_package(specifier) {
-      let file_path = specifier.to_file_path().unwrap();
+      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 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());
-        if let Some(referrer) = maybe_referrer {
+        if let Some(referrer) = &maybe_referrer {
           msg.push_str(" imported from ");
           msg.push_str(referrer.as_str());
         }
         msg
       })?;
-      let is_cjs = self.ps.cjs_resolutions.lock().contains(specifier);
 
-      let code = if is_cjs {
+      let code = if let Some(translate_kind) = maybe_translate_kind {
         // translate cjs to esm if it's cjs and inject node globals
         node::translate_cjs_to_esm(
           &self.ps.file_fetcher,
-          specifier,
+          &load_specifier,
           code,
           MediaType::Cjs,
           &self.ps.npm_resolver,
+          translate_kind,
         )?
       } else {
         // only inject node globals for esm
-        node::esm_code_with_node_globals(specifier, code)?
+        node::esm_code_with_node_globals(&load_specifier, code)?
       };
       ModuleCodeSource {
         code,

cli/node/mod.rs

@@ -255,6 +255,11 @@ 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)
 }
@@ -412,7 +417,15 @@ pub fn node_resolve(
     None => return Ok(None),
   };
 
-  let resolve_response = url_to_node_resolution(url, npm_resolver)?;
+  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,
+  };
   // TODO(bartlomieju): skipped checking errors for commonJS resolution and
   // "preserveSymlinksMain"/"preserveSymlinks" options.
   Ok(Some(resolve_response))
@@ -440,7 +453,15 @@ pub fn node_resolve_npm_reference(
   })?;
 
   let url = ModuleSpecifier::from_file_path(resolved_path).unwrap();
-  let resolve_response = url_to_node_resolution(url, npm_resolver)?;
+  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,
+  };
   // TODO(bartlomieju): skipped checking errors for commonJS resolution and
   // "preserveSymlinksMain"/"preserveSymlinks" options.
   Ok(Some(resolve_response))
@@ -701,6 +722,12 @@ 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.
 ///
@@ -713,6 +740,7 @@ 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,
@@ -734,7 +762,6 @@ pub fn translate_cjs_to_esm(
   let mut handled_reexports: HashSet<String> = HashSet::default();
 
   let mut source = vec![
-    r#"var window = undefined;"#.to_string(),
     r#"const require = Deno[Deno.internal].require.Module.createRequire(import.meta.url);"#.to_string(),
   ];
 
@@ -813,7 +840,9 @@ pub fn translate_cjs_to_esm(
   let mut had_default = false;
   for export in &all_exports {
     if export.as_str() == "default" {
-      if root_exports.contains("__esModule") {
+      if translate_kind == CjsToEsmTranslateKind::Deno
+        && root_exports.contains("__esModule")
+      {
         source.push(format!("export default mod[\"{}\"];", export));
         had_default = true;
       }

cli/tests/integration/npm_tests.rs

@@ -33,6 +33,13 @@ itest!(esm_module_deno_test {
   http_server: true,
 });
 
+itest!(esm_import_cjs_default {
+  args: "run --allow-read --allow-env --unstable --quiet npm/esm_import_cjs_default/main.js",
+  output: "npm/esm_import_cjs_default/main.out",
+  envs: env_vars(),
+  http_server: true,
+});
+
 itest!(cjs_with_deps {
   args: "run --allow-read --allow-env --unstable npm/cjs_with_deps/main.js",
   output: "npm/cjs_with_deps/main.out",

cli/tests/testdata/npm/esm_import_cjs_default/main.js

@@ -0,0 +1,22 @@
+import cjsDefault, {
+  MyClass as MyCjsClass,
+} from "npm:@denotest/cjs-default-export";
+import * as cjsNamespace from "npm:@denotest/cjs-default-export";
+import esmDefault from "npm:@denotest/esm-import-cjs-default";
+import * as esmNamespace from "npm:@denotest/esm-import-cjs-default";
+
+console.log("Deno esm importing node cjs");
+console.log("===========================");
+console.log(cjsDefault);
+console.log(cjsNamespace);
+console.log("===========================");
+
+console.log("Deno esm importing node esm");
+console.log("===========================");
+console.log(esmDefault);
+console.log(esmNamespace);
+console.log("===========================");
+
+console.log(cjsDefault());
+console.log(esmDefault());
+console.log(MyCjsClass.someStaticMethod());

cli/tests/testdata/npm/esm_import_cjs_default/main.out

@@ -0,0 +1,35 @@
+Node esm importing node cjs
+===========================
+{ default: [Function], named: [Function], MyClass: [Function: MyClass] }
+{ default: [Function], named: [Function] }
+Module {
+  MyClass: [Function: MyClass],
+  __esModule: true,
+  default: { default: [Function], named: [Function], MyClass: [Function: MyClass] },
+  named: [Function]
+}
+Module {
+  __esModule: true,
+  default: { default: [Function], named: [Function] },
+  named: [Function]
+}
+===========================
+static method
+Deno esm importing node cjs
+===========================
+[Function]
+Module {
+  MyClass: [Function: MyClass],
+  __esModule: true,
+  default: [Function],
+  named: [Function]
+}
+===========================
+Deno esm importing node esm
+===========================
+[Function: default]
+Module { default: [Function: default] }
+===========================
+1
+5
+static method

cli/tests/testdata/npm/registry/@denotest/cjs-default-export/1.0.0/index.js

@@ -0,0 +1,17 @@
+Object.defineProperty(module.exports, "__esModule", {
+  value: true
+});
+module.exports["default"] = function() {
+  return 1;
+};
+module.exports["named"] = function() {
+  return 2;
+};
+
+class MyClass {
+  static someStaticMethod() {
+    return "static method";
+  }
+}
+
+module.exports.MyClass = MyClass;

cli/tests/testdata/npm/registry/@denotest/cjs-default-export/1.0.0/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@denotest/cjs-default-export",
+  "version": "1.0.0"
+}

cli/tests/testdata/npm/registry/@denotest/esm-import-cjs-default/1.0.0/index.mjs

@@ -0,0 +1,17 @@
+import defaultImport, { MyClass } from "@denotest/cjs-default-export";
+import * as namespaceImport from "@denotest/cjs-default-export";
+import localDefaultImport from "./local.cjs";
+import * as localNamespaceImport from "./local.cjs";
+
+console.log("Node esm importing node cjs");
+console.log("===========================");
+console.log(defaultImport);
+console.log(localDefaultImport);
+console.log(namespaceImport);
+console.log(localNamespaceImport);
+console.log("===========================");
+console.log(MyClass.someStaticMethod());
+
+export default function() {
+  return defaultImport.default() * 5;
+}

cli/tests/testdata/npm/registry/@denotest/esm-import-cjs-default/1.0.0/local.cjs

@@ -0,0 +1,9 @@
+Object.defineProperty(module.exports, "__esModule", {
+  value: true
+});
+module.exports["default"] = function() {
+  return 3;
+};
+module.exports["named"] = function() {
+  return 4;
+};

cli/tests/testdata/npm/registry/@denotest/esm-import-cjs-default/1.0.0/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "@denotest/esm-import-cjs-default",
+  "version": "1.0.0",
+  "dependencies": {
+    "@denotest/cjs-default-export": "^1.0.0"
+  }
+}