fix(npm): recursive translation of reexports, remove window global in node code (#15806)

Co-authored-by: David Sherret <dsherret@gmail.com>

Cargo.lock

@@ -1142,6 +1142,7 @@ name = "deno_node"
 version = "0.4.0"
 dependencies = [
  "deno_core",
+ "once_cell",
  "path-clean",
  "regex",
  "serde",

cli/node/analyze.rs

@@ -9,6 +9,7 @@ use deno_ast::ModuleSpecifier;
 use deno_ast::ParsedSource;
 use deno_ast::SourceRanged;
 use deno_core::error::AnyError;
+use deno_runtime::deno_node::NODE_GLOBAL_THIS_NAME;
 use std::fmt::Write;
 
 static NODE_GLOBALS: &[&str] = &[
@@ -52,18 +53,7 @@ pub fn esm_code_with_node_globals(
   }
 
   let mut result = String::new();
-  let has_deno_decl = top_level_decls.contains("Deno");
-  let global_this_expr = if has_deno_decl {
-    if top_level_decls.contains("window") {
-      // Will probably never happen, but if it does then we should consider
-      // creating an obscure global name to get this from.
-      panic!("The node esm module had a local `Deno` declaration and `window` declaration.");
-    }
-    // fallback to using `window.Deno`
-    "window.Deno[Deno.internal].node.globalThis"
-  } else {
-    "Deno[Deno.internal].node.globalThis"
-  };
+  let global_this_expr = NODE_GLOBAL_THIS_NAME.as_str();
   let global_this_expr = if has_global_this {
     global_this_expr
   } else {
@@ -162,7 +152,10 @@ mod tests {
       "export const x = 1;".to_string(),
     )
     .unwrap();
-    assert!(r.contains("var globalThis = Deno[Deno.internal].node.globalThis;"));
+    assert!(r.contains(&format!(
+      "var globalThis = {};",
+      NODE_GLOBAL_THIS_NAME.as_str()
+    )));
     assert!(r.contains("var process = globalThis.process;"));
     assert!(r.contains("export const x = 1;"));
   }
@@ -176,14 +169,18 @@ mod tests {
     .unwrap();
     assert_eq!(
       r,
+      format!(
         concat!(
-        "var globalThis = Deno[Deno.internal].node.globalThis;var Buffer = globalThis.Buffer;",
+          "var globalThis = {}",
+          ";var Buffer = globalThis.Buffer;",
           "var clearImmediate = globalThis.clearImmediate;var clearInterval = globalThis.clearInterval;",
           "var clearTimeout = globalThis.clearTimeout;var global = globalThis.global;",
           "var process = globalThis.process;var setImmediate = globalThis.setImmediate;",
           "var setInterval = globalThis.setInterval;var setTimeout = globalThis.setTimeout;\n",
           "export const x = 1;"
         ),
+        NODE_GLOBAL_THIS_NAME.as_str(),
+      )
     );
   }
 }

cli/node/mod.rs

@@ -1,10 +1,12 @@
 // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license.
 
 use std::collections::HashSet;
+use std::collections::VecDeque;
 use std::path::Path;
 use std::path::PathBuf;
 
 use crate::deno_std::CURRENT_STD_URL;
+use deno_ast::CjsAnalysis;
 use deno_ast::MediaType;
 use deno_ast::ModuleSpecifier;
 use deno_core::anyhow::bail;
@@ -25,6 +27,7 @@ use deno_runtime::deno_node::DenoDirNpmResolver;
 use deno_runtime::deno_node::NodeModuleKind;
 use deno_runtime::deno_node::PackageJson;
 use deno_runtime::deno_node::DEFAULT_CONDITIONS;
+use deno_runtime::deno_node::NODE_GLOBAL_THIS_NAME;
 use once_cell::sync::Lazy;
 use path_clean::PathClean;
 use regex::Regex;
@@ -326,11 +329,12 @@ pub async fn initialize_runtime(
   js_runtime: &mut JsRuntime,
 ) -> Result<(), AnyError> {
   let source_code = &format!(
-    r#"(async function loadBuiltinNodeModules(moduleAllUrl) {{
+    r#"(async function loadBuiltinNodeModules(moduleAllUrl, nodeGlobalThisName) {{
       const moduleAll = await import(moduleAllUrl);
-      Deno[Deno.internal].node.initialize(moduleAll.default);
-    }})('{}');"#,
+      Deno[Deno.internal].node.initialize(moduleAll.default, nodeGlobalThisName);
+    }})('{}', '{}');"#,
     MODULE_ALL_URL.as_str(),
+    NODE_GLOBAL_THIS_NAME.as_str(),
   );
 
   let value =
@@ -710,6 +714,11 @@ pub fn translate_cjs_to_esm(
   media_type: MediaType,
   npm_resolver: &GlobalNpmPackageResolver,
 ) -> Result<String, AnyError> {
+  fn perform_cjs_analysis(
+    specifier: &str,
+    media_type: MediaType,
+    code: String,
+  ) -> Result<CjsAnalysis, AnyError> {
     let parsed_source = deno_ast::parse_script(deno_ast::ParseParams {
       specifier: specifier.to_string(),
       text_info: deno_ast::SourceTextInfo::new(code.into()),
@@ -718,24 +727,47 @@ pub fn translate_cjs_to_esm(
       scope_analysis: false,
       maybe_syntax: None,
     })?;
-  let analysis = parsed_source.analyze_cjs();
+    Ok(parsed_source.analyze_cjs())
+  }
+
+  let mut temp_var_count = 0;
+  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(),
+  ];
+
+  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 temp_var_count = 0;
+  let mut all_exports = analysis
+    .exports
+    .iter()
+    .map(|s| s.to_string())
+    .collect::<HashSet<_>>();
 
-  let mut source = vec![
-    r#"const require = Deno[Deno.internal].require.Module.createRequire(import.meta.url);"#.to_string(),
-  ];
+  // (request, referrer)
+  let mut reexports_to_handle = VecDeque::new();
+  for reexport in analysis.reexports {
+    reexports_to_handle.push_back((reexport, specifier.clone()));
+  }
 
-  // if there are reexports, handle them first
-  for (idx, reexport) in analysis.reexports.iter().enumerate() {
-    // Firstly, resolve relate reexport specifier
+  while let Some((reexport, referrer)) = reexports_to_handle.pop_front() {
+    if handled_reexports.contains(&reexport) {
+      continue;
+    }
+
+    handled_reexports.insert(reexport.to_string());
+
+    // First, resolve relate reexport specifier
     let resolved_reexport = resolve(
-      reexport,
-      specifier,
+      &reexport,
+      &referrer,
       // FIXME(bartlomieju): check if these conditions are okay, probably
       // should be `deno-require`, because `deno` is already used in `esm_resolver.rs`
       &["deno", "require", "default"],
@@ -743,35 +775,26 @@ pub fn translate_cjs_to_esm(
     )?;
     let reexport_specifier =
       ModuleSpecifier::from_file_path(&resolved_reexport).unwrap();
-    // Secondly, read the source code from disk
+    // Second, read the source code from disk
     let reexport_file = file_fetcher.get_source(&reexport_specifier).unwrap();
-    // Now perform analysis again
+
     {
-      let parsed_source = deno_ast::parse_script(deno_ast::ParseParams {
-        specifier: reexport_specifier.to_string(),
-        text_info: deno_ast::SourceTextInfo::new(reexport_file.source),
-        media_type: reexport_file.media_type,
-        capture_tokens: true,
-        scope_analysis: false,
-        maybe_syntax: None,
-      })?;
-      let analysis = parsed_source.analyze_cjs();
+      let analysis = perform_cjs_analysis(
+        reexport_specifier.as_str(),
+        reexport_file.media_type,
+        reexport_file.source.to_string(),
+      )?;
 
-      source.push(format!(
-        "const reexport{} = require(\"{}\");",
-        idx, reexport
-      ));
-
-      for export in analysis.exports.iter().filter(|e| {
-        e.as_str() != "default" && !root_exports.contains(e.as_str())
-      }) {
-        add_export(
-          &mut source,
-          export,
-          &format!("Deno[Deno.internal].require.bindExport(reexport{0}[\"{1}\"], reexport{0})", idx, export),
-          &mut temp_var_count,
-        );
+      for reexport in analysis.reexports {
+        reexports_to_handle.push_back((reexport, reexport_specifier.clone()));
       }
+
+      all_exports.extend(
+        analysis
+          .exports
+          .into_iter()
+          .filter(|e| e.as_str() != "default"),
+      );
     }
   }
 
@@ -788,7 +811,7 @@ pub fn translate_cjs_to_esm(
   ));
 
   let mut had_default = false;
-  for export in analysis.exports.iter() {
+  for export in &all_exports {
     if export.as_str() == "default" {
       if root_exports.contains("__esModule") {
         source.push(format!(
@@ -880,7 +903,6 @@ fn resolve(
       return Ok(module_dir.join("index.js").clean());
     }
   }
-
   Err(not_found(specifier, &referrer_path))
 }
 
@@ -988,7 +1010,6 @@ fn file_extension_probe(
       return Ok(p_js);
     }
   }
-
   Err(not_found(&p.to_string_lossy(), referrer))
 }
 

cli/tests/integration/npm_tests.rs

@@ -89,13 +89,33 @@ itest!(dual_cjs_esm {
   http_server: true,
 });
 
-itest!(dynamic_import {
-  args: "run --allow-read --allow-env --unstable npm/dynamic_import/main.ts",
-  output: "npm/dynamic_import/main.out",
+// FIXME(bartlomieju): npm: specifiers are not handled in dynamic imports
+// at the moment
+// itest!(dynamic_import {
+//   args: "run --allow-read --allow-env --unstable npm/dynamic_import/main.ts",
+//   output: "npm/dynamic_import/main.out",
+//   envs: env_vars(),
+//   http_server: true,
+// });
+
+itest!(env_var_re_export_dev {
+  args: "run --allow-read --allow-env --unstable --quiet npm/env_var_re_export/main.js",
+  output_str: Some("dev\n"),
   envs: env_vars(),
   http_server: true,
 });
 
+itest!(env_var_re_export_prod {
+  args: "run --allow-read --allow-env --unstable --quiet npm/env_var_re_export/main.js",
+  output_str: Some("prod\n"),
+  envs: {
+    let mut vars = env_vars();
+    vars.push(("NODE_ENV".to_string(), "production".to_string()));
+    vars
+  },
+  http_server: true,
+});
+
 itest!(cached_only {
   args: "run --cached-only --unstable npm/cached_only/main.ts",
   output: "npm/cached_only/main.out",

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

@@ -0,0 +1,3 @@
+import { getEnv } from "npm:@denotest/env-var-re-export";
+
+console.log(getEnv());

cli/tests/testdata/npm/registry/@denotest/env-var-re-export/1.0.0/dev.cjs

@@ -0,0 +1,5 @@
+module.exports = {
+  getEnv() {
+    return "dev";
+  },
+};

cli/tests/testdata/npm/registry/@denotest/env-var-re-export/1.0.0/index.cjs

@@ -0,0 +1,5 @@
+if (process.env.NODE_ENV === 'production') {
+  module.exports = require('./prod.cjs');
+} else {
+  module.exports = require('./dev.cjs');
+}

cli/tests/testdata/npm/registry/@denotest/env-var-re-export/1.0.0/package.json

@@ -0,0 +1,5 @@
+{
+  "name": "@denotest/env-var-re-export",
+  "version": "1.0.0",
+  "main": "./index.cjs"
+}

cli/tests/testdata/npm/registry/@denotest/env-var-re-export/1.0.0/prod.cjs

@@ -0,0 +1,5 @@
+module.exports = {
+  getEnv() {
+    return "prod";
+  },
+};

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

@@ -1,2 +1,5 @@
 import fsx from "npm:fs-extra@10.1.0";
+import { createApp } from "npm:vue";
+
 console.log(fsx.access);
+console.log(createApp);

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

@@ -1 +1,2 @@
 [Function: access]
+[Function: bound createApp]

ext/node/01_node.js

@@ -10,6 +10,7 @@
     ArrayPrototypeFilter,
     ObjectEntries,
     ObjectCreate,
+    ObjectDefineProperty,
   } = window.__bootstrap.primordials;
 
   function assert(cond) {
@@ -82,7 +83,7 @@
   const nativeModuleExports = ObjectCreate(null);
   const builtinModules = [];
 
-  function initialize(nodeModules) {
+  function initialize(nodeModules, nodeGlobalThisName) {
     assert(!initialized);
     initialized = true;
     for (const [name, exports] of ObjectEntries(nodeModules)) {
@@ -98,6 +99,14 @@
     nodeGlobals.setImmediate = nativeModuleExports["timers"].setImmediate;
     nodeGlobals.setInterval = nativeModuleExports["timers"].setInterval;
     nodeGlobals.setTimeout = nativeModuleExports["timers"].setTimeout;
+
+    // add a hidden global for the esm code to use in order to reliably
+    // get node's globalThis
+    ObjectDefineProperty(globalThis, nodeGlobalThisName, {
+      enumerable: false,
+      writable: false,
+      value: nodeGlobalThis,
+    });
   }
 
   window.__bootstrap.internals = {

ext/node/02_require.js

@@ -658,7 +658,7 @@
   Module.wrapper = [
     // We provide the non-standard APIs in the CommonJS wrapper
     // to avoid exposing them in global namespace.
-    "(function (exports, require, module, __filename, __dirname, globalThis) { const { Buffer, clearImmediate, clearInterval, clearTimeout, global, process, setImmediate, setInterval, setTimeout} = globalThis; (function () {",
+    "(function (exports, require, module, __filename, __dirname, globalThis) { const { Buffer, clearImmediate, clearInterval, clearTimeout, global, process, setImmediate, setInterval, setTimeout} = globalThis; var window = undefined; (function () {",
     "\n}).call(this); })",
   ];
   Module.wrap = function (script) {

ext/node/Cargo.toml

@@ -15,6 +15,7 @@ path = "lib.rs"
 
 [dependencies]
 deno_core = { version = "0.149.0", path = "../../core" }
+once_cell = "1.12.0"
 path-clean = "=0.1.0"
 regex = "1"
 serde = "1.0.136"

ext/node/lib.rs

@@ -7,6 +7,7 @@ use deno_core::op;
 use deno_core::url::Url;
 use deno_core::Extension;
 use deno_core::OpState;
+use once_cell::sync::Lazy;
 use std::path::Path;
 use std::path::PathBuf;
 use std::rc::Rc;
@@ -48,6 +49,16 @@ pub trait DenoDirNpmResolver {
 
 pub const MODULE_ES_SHIM: &str = include_str!("./module_es_shim.js");
 
+pub static NODE_GLOBAL_THIS_NAME: Lazy<String> = Lazy::new(|| {
+  let now = std::time::SystemTime::now();
+  let seconds = now
+    .duration_since(std::time::SystemTime::UNIX_EPOCH)
+    .unwrap()
+    .as_secs();
+  // use a changing variable name to make it hard to depend on this
+  format!("__DENO_NODE_GLOBAL_THIS_{}__", seconds)
+});
+
 struct Unstable(pub bool);
 
 pub fn init<P: NodePermissions + 'static>(