Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compat: support compat mode in REPL #12882

Merged
merged 11 commits into from
Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions cli/compat/esm_resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ impl NodeEsmResolver {
maybe_import_map_resolver,
}
}

pub fn as_resolver(&self) -> &dyn Resolver {
self
}
}

impl Resolver for NodeEsmResolver {
Expand Down
18 changes: 18 additions & 0 deletions cli/compat/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"\'")
}
1 change: 1 addition & 0 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?;

Expand Down
84 changes: 52 additions & 32 deletions cli/proc_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Arc<dyn deno_graph::source::Resolver + Send + Sync>>,
}

impl Deref for ProcState {
Expand Down Expand Up @@ -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<dyn deno_graph::source::Resolver + Send + Sync>,
> = 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,
Expand All @@ -328,6 +357,7 @@ impl ProcState {
broadcast_channel,
shared_array_buffer_store,
compiled_wasm_module_store,
maybe_resolver,
})))
}

Expand Down Expand Up @@ -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
};
Comment on lines +428 to +433
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Longhand version of saying this?

Suggested change
let maybe_resolver: Option<&dyn deno_graph::source::Resolver> =
if let Some(resolver) = &self.maybe_resolver {
Some(resolver.as_ref())
} else {
None
};
let maybe_resolver: Option<&dyn deno_graph::source::Resolver> =
self.maybe_resolver.as_ref();

(Or maybe .as_deref(). Likewise around line 661.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that before but it doesn't work:

error[E0308]: mismatched types
   --> cli/proc_state.rs:429:7
    |
428 |     let maybe_resolver: Option<&dyn deno_graph::source::Resolver> =
    |                         ----------------------------------------- expected due to this
429 |       self.maybe_resolver.as_ref();
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected trait object `dyn Resolver`, found struct `Arc`
    |
    = note: expected enum `std::option::Option<&dyn Resolver>`
               found enum `std::option::Option<&Arc<dyn Resolver + std::marker::Send + std::marker::Sync>>`

I had to add additional trait boundaries (Send + Sync) to the dyn Resolver and found that it's the only method to get it past type checking.

let graph = create_graph(
roots.clone(),
is_dynamic,
Expand Down Expand Up @@ -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()
};
Comment on lines +652 to 659
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I'm happy about this solution, but it's required because deno_graph::source::Resolver requires &Url type for referrer arg

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not wholly clear to me why it needs to be converted to &dyn Resolver first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by converting - but we're using dyn Resolver to have a single interface from deno_graph; that can be used in prepare_module_load method and resolve methods on the ProcState.

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())
}
}

Expand Down
22 changes: 21 additions & 1 deletion cli/tests/integration/compat_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ itest!(compat_dyn_import_rejects_with_node_compatible_error {
output: "compat/dyn_import_reject.out",
});

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(
Expand All @@ -40,6 +46,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(
Expand All @@ -54,5 +74,5 @@ fn node_compat_url() {
);
assert!(out.is_empty());
assert!(!err.is_empty());
assert!(err.contains("file:///non_existent/node/global.ts"));
assert!(err.contains("non_existent/node/global.ts"));
}
2 changes: 1 addition & 1 deletion cli/tests/testdata/compat/dyn_import_reject.out
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
TypeError: Cannot load module "file:///[WILDCARD]/testdata/compat/foobar.js".
TypeError: [ERR_MODULE_NOT_FOUND] Cannot find module '[WILDCARD]/testdata/compat/foobar.js' imported from file:///[WILDCARD]/testdata/compat/dyn_import_reject.js
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
ERR_MODULE_NOT_FOUND
1 change: 1 addition & 0 deletions cli/tests/testdata/compat/import_esm_from_cjs/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require("pure-cjs");

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.