From 1117d2db3965fc44f174be3fd029293b7e2b952e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Wed, 24 Nov 2021 16:55:10 +0100 Subject: [PATCH] compat: support compat mode in REPL (#12882) This commit introduces "ProcState::maybe_resolver" field, which stores a single instance of resolver for the whole lifetime of the process, instead of creating these resolvers for each creation of module graph. As a result, this resolver can be used in fallback case where graph is not constructed (REPL, loading modules using "require") unifying resolution logic. --- cli/compat/errors.rs | 2 +- cli/compat/esm_resolver.rs | 10 +-- cli/compat/mod.rs | 18 ++++ cli/main.rs | 1 + cli/proc_state.rs | 84 ++++++++++++------- cli/tests/integration/compat_tests.rs | 20 +++++ .../testdata/compat/dyn_import_reject.out | 2 +- .../compat/import_esm_from_cjs/index.js | 1 + .../node_modules/pure-cjs/index.js | 4 + .../node_modules/pure-cjs/package.json | 4 + .../node_modules/pure-esm/index.js | 2 + .../node_modules/pure-esm/package.json | 5 ++ 12 files changed, 112 insertions(+), 41 deletions(-) create mode 100644 cli/tests/testdata/compat/import_esm_from_cjs/index.js create mode 100644 cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-cjs/index.js create mode 100644 cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-cjs/package.json create mode 100644 cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-esm/index.js create mode 100644 cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-esm/package.json diff --git a/cli/compat/errors.rs b/cli/compat/errors.rs index 3a44b2c88f..e9cc31a0db 100644 --- a/cli/compat/errors.rs +++ b/cli/compat/errors.rs @@ -49,7 +49,7 @@ pub(crate) fn err_module_not_found( typ: &str, ) -> AnyError { generic_error(format!( - "[ERR_MODULE_NOT_FOUND] Cannot find {} '{}' imported from {}", + "[ERR_MODULE_NOT_FOUND] Cannot find {} \"{}\" imported from \"{}\"", typ, path, base )) } diff --git a/cli/compat/esm_resolver.rs b/cli/compat/esm_resolver.rs index 980e44b32b..5787c9e485 100644 --- a/cli/compat/esm_resolver.rs +++ b/cli/compat/esm_resolver.rs @@ -24,10 +24,6 @@ impl NodeEsmResolver { maybe_import_map_resolver, } } - - pub fn as_resolver(&self) -> &dyn Resolver { - self - } } impl Resolver for NodeEsmResolver { @@ -232,12 +228,12 @@ fn finalize_resolution( }; if is_dir { return Err(errors::err_unsupported_dir_import( - &path.display().to_string(), - &to_file_path_string(base), + resolved.as_str(), + base.as_str(), )); } else if !is_file { return Err(errors::err_module_not_found( - &path.display().to_string(), + resolved.as_str(), base.as_str(), "module", )); diff --git a/cli/compat/mod.rs b/cli/compat/mod.rs index 01d84d65a5..41443968f8 100644 --- a/cli/compat/mod.rs +++ b/cli/compat/mod.rs @@ -104,6 +104,24 @@ pub(crate) fn load_cjs_module( Ok(()) } +pub(crate) fn add_global_require( + js_runtime: &mut JsRuntime, + main_module: &str, +) -> Result<(), AnyError> { + let source_code = &format!( + r#"(async function setupGlobalRequire(main) {{ + const Module = await import("{}"); + const require = Module.createRequire(main); + globalThis.require = require; + }})('{}');"#, + MODULE_URL_STR.as_str(), + escape_for_single_quote_string(main_module), + ); + + js_runtime.execute_script(&located_script_name!(), source_code)?; + Ok(()) +} + fn escape_for_single_quote_string(text: &str) -> String { text.replace(r"\", r"\\").replace("'", r"\'") } diff --git a/cli/main.rs b/cli/main.rs index 33895ec750..87fe284611 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -948,6 +948,7 @@ async fn run_repl(flags: Flags, repl_flags: ReplFlags) -> Result<(), AnyError> { create_main_worker(&ps, main_module.clone(), permissions, None); if flags.compat { worker.execute_side_module(&compat::GLOBAL_URL).await?; + compat::add_global_require(&mut worker.js_runtime, main_module.as_str())?; } worker.run_event_loop(false).await?; diff --git a/cli/proc_state.rs b/cli/proc_state.rs index c8e3968d20..7615b9d464 100644 --- a/cli/proc_state.rs +++ b/cli/proc_state.rs @@ -167,6 +167,7 @@ pub struct Inner { pub broadcast_channel: InMemoryBroadcastChannel, pub shared_array_buffer_store: SharedArrayBufferStore, pub compiled_wasm_module_store: CompiledWasmModuleStore, + maybe_resolver: Option>, } impl Deref for ProcState { @@ -313,6 +314,34 @@ impl ProcState { .clone() .or_else(|| env::var("DENO_UNSTABLE_COVERAGE_DIR").ok()); + // FIXME(bartlomieju): `NodeEsmResolver` is not aware of JSX resolver + // created below + let node_resolver = NodeEsmResolver::new( + maybe_import_map.clone().map(ImportMapResolver::new), + ); + let maybe_import_map_resolver = + maybe_import_map.clone().map(ImportMapResolver::new); + let maybe_jsx_resolver = maybe_config_file + .as_ref() + .map(|cf| { + cf.to_maybe_jsx_import_source_module() + .map(|im| JsxResolver::new(im, maybe_import_map_resolver.clone())) + }) + .flatten(); + let maybe_resolver: Option< + Arc, + > = if flags.compat { + Some(Arc::new(node_resolver)) + } else if let Some(jsx_resolver) = maybe_jsx_resolver { + // the JSX resolver offloads to the import map if present, otherwise uses + // the default Deno explicit import resolution. + Some(Arc::new(jsx_resolver)) + } else if let Some(import_map_resolver) = maybe_import_map_resolver { + Some(Arc::new(import_map_resolver)) + } else { + None + }; + Ok(ProcState(Arc::new(Inner { dir, coverage_dir, @@ -328,6 +357,7 @@ impl ProcState { broadcast_channel, shared_array_buffer_store, compiled_wasm_module_store, + maybe_resolver, }))) } @@ -395,30 +425,12 @@ impl ProcState { ); let maybe_locker = as_maybe_locker(self.lockfile.clone()); let maybe_imports = self.get_maybe_imports()?; - let node_resolver = NodeEsmResolver::new( - self.maybe_import_map.clone().map(ImportMapResolver::new), - ); - let maybe_import_map_resolver = - self.maybe_import_map.clone().map(ImportMapResolver::new); - let maybe_jsx_resolver = self - .maybe_config_file - .as_ref() - .map(|cf| { - cf.to_maybe_jsx_import_source_module() - .map(|im| JsxResolver::new(im, maybe_import_map_resolver.clone())) - }) - .flatten(); - let maybe_resolver = if self.flags.compat { - Some(node_resolver.as_resolver()) - } else if maybe_jsx_resolver.is_some() { - // the JSX resolver offloads to the import map if present, otherwise uses - // the default Deno explicit import resolution. - maybe_jsx_resolver.as_ref().map(|jr| jr.as_resolver()) - } else { - maybe_import_map_resolver - .as_ref() - .map(|im| im.as_resolver()) - }; + let maybe_resolver: Option<&dyn deno_graph::source::Resolver> = + if let Some(resolver) = &self.maybe_resolver { + Some(resolver.as_ref()) + } else { + None + }; let graph = create_graph( roots.clone(), is_dynamic, @@ -637,18 +649,26 @@ impl ProcState { } } - // FIXME(bartlomieju): hacky way to provide compatibility with repl + // FIXME(bartlomieju): this is a hacky way to provide compatibility with REPL + // and `Deno.core.evalContext` API. Ideally we should always have a referrer filled + // but sadly that's not the case due to missing APIs in V8. let referrer = if referrer.is_empty() && self.flags.repl { - deno_core::DUMMY_SPECIFIER + deno_core::resolve_url_or_path("./$deno$repl.ts").unwrap() } else { - referrer + deno_core::resolve_url_or_path(referrer).unwrap() }; - if let Some(import_map) = &self.maybe_import_map { - import_map - .resolve(specifier, referrer) - .map_err(|err| err.into()) + + let maybe_resolver: Option<&dyn deno_graph::source::Resolver> = + if let Some(resolver) = &self.maybe_resolver { + Some(resolver.as_ref()) + } else { + None + }; + if let Some(resolver) = &maybe_resolver { + resolver.resolve(specifier, &referrer) } else { - deno_core::resolve_import(specifier, referrer).map_err(|err| err.into()) + deno_core::resolve_import(specifier, referrer.as_str()) + .map_err(|err| err.into()) } } diff --git a/cli/tests/integration/compat_tests.rs b/cli/tests/integration/compat_tests.rs index 70c224e311..fdd9813f08 100644 --- a/cli/tests/integration/compat_tests.rs +++ b/cli/tests/integration/compat_tests.rs @@ -46,6 +46,12 @@ itest!(compat_dyn_import_rejects_with_node_compatible_error { envs: vec![("DENO_NODE_COMPAT_URL".to_string(), std_file_url())], }); +itest!(import_esm_from_cjs { + args: + "run --compat --unstable -A --quiet compat/import_esm_from_cjs/index.js", + output_str: Some("function\n"), +}); + #[test] fn globals_in_repl() { let (out, _err) = util::run_and_collect_output_with_args( @@ -58,6 +64,20 @@ fn globals_in_repl() { assert!(out.contains("true")); } +#[test] +fn require_in_repl() { + let (out, _err) = util::run_and_collect_output_with_args( + true, + vec!["repl", "--compat", "--unstable", "--quiet"], + Some(vec![ + "const foo = require('./compat/import_esm_from_cjs/index');", + ]), + None, + false, + ); + assert!(out.contains("function")); +} + #[test] fn node_compat_url() { let (out, err) = util::run_and_collect_output_with_args( diff --git a/cli/tests/testdata/compat/dyn_import_reject.out b/cli/tests/testdata/compat/dyn_import_reject.out index 6d78135b27..1324c8aeb2 100644 --- a/cli/tests/testdata/compat/dyn_import_reject.out +++ b/cli/tests/testdata/compat/dyn_import_reject.out @@ -1,2 +1,2 @@ -TypeError: Cannot load module "file:///[WILDCARD]/testdata/compat/foobar.js". +TypeError: [ERR_MODULE_NOT_FOUND] Cannot find module "file://[WILDCARD]/testdata/compat/foobar.js" imported from "file://[WILDCARD]/testdata/compat/dyn_import_reject.js" ERR_MODULE_NOT_FOUND diff --git a/cli/tests/testdata/compat/import_esm_from_cjs/index.js b/cli/tests/testdata/compat/import_esm_from_cjs/index.js new file mode 100644 index 0000000000..4ba03e1045 --- /dev/null +++ b/cli/tests/testdata/compat/import_esm_from_cjs/index.js @@ -0,0 +1 @@ +require("pure-cjs"); diff --git a/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-cjs/index.js b/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-cjs/index.js new file mode 100644 index 0000000000..35f7c37744 --- /dev/null +++ b/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-cjs/index.js @@ -0,0 +1,4 @@ +async function run() { + const _result = await import('pure-esm'); +} +run() diff --git a/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-cjs/package.json b/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-cjs/package.json new file mode 100644 index 0000000000..e854fd9926 --- /dev/null +++ b/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-cjs/package.json @@ -0,0 +1,4 @@ +{ + "name": "pure-cjs", + "main": "./index.js" +} diff --git a/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-esm/index.js b/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-esm/index.js new file mode 100644 index 0000000000..898097cb5a --- /dev/null +++ b/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-esm/index.js @@ -0,0 +1,2 @@ +import fs from 'node:fs'; +console.log(typeof fs.chmod); diff --git a/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-esm/package.json b/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-esm/package.json new file mode 100644 index 0000000000..a373d3ad98 --- /dev/null +++ b/cli/tests/testdata/compat/import_esm_from_cjs/node_modules/pure-esm/package.json @@ -0,0 +1,5 @@ +{ + "name": "pure-esm", + "type": "module", + "main": "./index.js" +}