Skip to content

Commit

Permalink
fix(core): allow esm extensions not included in snapshot (denoland#18980
Browse files Browse the repository at this point in the history
)

Fixes denoland#18979.

This changes the predicate for allowing `ext:` specifier resolution from
`snapshot_loaded_and_not_snapshotting` to `ext_resolution_allowed` which
is only set to true during the extension module loading phase. Module
loaders as used in core
are now declared as `ExtModuleLoader` rather than `dyn ModuleLoader`.
  • Loading branch information
nayeemrmn committed May 4, 2023
1 parent e3276fb commit 7a8bb3b
Show file tree
Hide file tree
Showing 9 changed files with 219 additions and 363 deletions.
4 changes: 1 addition & 3 deletions bench_util/js_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ use crate::profiling::is_profiling;
pub fn create_js_runtime(setup: impl FnOnce() -> Vec<Extension>) -> JsRuntime {
JsRuntime::new(RuntimeOptions {
extensions: setup(),
module_loader: Some(
std::rc::Rc::new(deno_core::ExtModuleLoader::default()),
),
module_loader: None,
..Default::default()
})
}
Expand Down
2 changes: 1 addition & 1 deletion cli/tests/testdata/run/extension_dynamic_import.ts.out
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: Uncaught TypeError: Cannot load extension module from external code
error: Uncaught (in promise) TypeError: Cannot load extension module from external code
await import("ext:runtime/01_errors.js");
^
at [WILDCARD]/extension_dynamic_import.ts:1:1
91 changes: 33 additions & 58 deletions core/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use crate::error::is_instance_of_error;
use crate::error::JsStackFrame;
use crate::modules::get_asserted_module_type_from_assertions;
use crate::modules::parse_import_assertions;
use crate::modules::resolve_helper;
use crate::modules::validate_import_assertions;
use crate::modules::ImportAssertionsKind;
use crate::modules::ModuleMap;
Expand Down Expand Up @@ -259,46 +258,43 @@ pub fn host_import_module_dynamically_callback<'s>(
.unwrap()
.to_rust_string_lossy(scope);

let is_ext_module = specifier_str.starts_with("ext:");
let resolver = v8::PromiseResolver::new(scope).unwrap();
let promise = resolver.get_promise(scope);

if !is_ext_module {
let assertions = parse_import_assertions(
scope,
import_assertions,
ImportAssertionsKind::DynamicImport,
);
let assertions = parse_import_assertions(
scope,
import_assertions,
ImportAssertionsKind::DynamicImport,
);

{
let tc_scope = &mut v8::TryCatch::new(scope);
validate_import_assertions(tc_scope, &assertions);
if tc_scope.has_caught() {
let e = tc_scope.exception().unwrap();
resolver.reject(tc_scope, e);
}
{
let tc_scope = &mut v8::TryCatch::new(scope);
validate_import_assertions(tc_scope, &assertions);
if tc_scope.has_caught() {
let e = tc_scope.exception().unwrap();
resolver.reject(tc_scope, e);
}
let asserted_module_type =
get_asserted_module_type_from_assertions(&assertions);
}
let asserted_module_type =
get_asserted_module_type_from_assertions(&assertions);

let resolver_handle = v8::Global::new(scope, resolver);
{
let state_rc = JsRuntime::state(scope);
let module_map_rc = JsRuntime::module_map(scope);
let resolver_handle = v8::Global::new(scope, resolver);
{
let state_rc = JsRuntime::state(scope);
let module_map_rc = JsRuntime::module_map(scope);

debug!(
"dyn_import specifier {} referrer {} ",
specifier_str, referrer_name_str
);
ModuleMap::load_dynamic_import(
module_map_rc,
&specifier_str,
&referrer_name_str,
asserted_module_type,
resolver_handle,
);
state_rc.borrow_mut().notify_new_dynamic_import();
}
debug!(
"dyn_import specifier {} referrer {} ",
specifier_str, referrer_name_str
);
ModuleMap::load_dynamic_import(
module_map_rc,
&specifier_str,
&referrer_name_str,
asserted_module_type,
resolver_handle,
);
state_rc.borrow_mut().notify_new_dynamic_import();
}
// Map errors from module resolution (not JS errors from module execution) to
// ones rethrown from this scope, so they include the call stack of the
Expand All @@ -311,16 +307,6 @@ pub fn host_import_module_dynamically_callback<'s>(

let promise = promise.catch(scope, map_err).unwrap();

if is_ext_module {
let message = v8::String::new_external_onebyte_static(
scope,
b"Cannot load extension module from external code",
)
.unwrap();
let exception = v8::Exception::type_error(scope, message);
resolver.reject(scope, exception);
}

Some(promise)
}

Expand Down Expand Up @@ -375,27 +361,16 @@ fn import_meta_resolve(
url_prop.to_rust_string_lossy(scope)
};
let module_map_rc = JsRuntime::module_map(scope);
let (loader, snapshot_loaded_and_not_snapshotting) = {
let module_map = module_map_rc.borrow();
(
module_map.loader.clone(),
module_map.snapshot_loaded_and_not_snapshotting,
)
};
let loader = module_map_rc.borrow().loader.clone();
let specifier_str = specifier.to_rust_string_lossy(scope);

if specifier_str.starts_with("npm:") {
throw_type_error(scope, "\"npm:\" specifiers are currently not supported in import.meta.resolve()");
return;
}

match resolve_helper(
snapshot_loaded_and_not_snapshotting,
loader,
&specifier_str,
&referrer,
ResolutionKind::DynamicImport,
) {
match loader.resolve(&specifier_str, &referrer, ResolutionKind::DynamicImport)
{
Ok(resolved) => {
let resolved_val = serde_v8::to_v8(scope, resolved.as_str()).unwrap();
rv.set(resolved_val);
Expand Down
10 changes: 10 additions & 0 deletions core/extensions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,16 @@ impl Extension {
pub fn disable(self) -> Self {
self.enabled(false)
}

pub(crate) fn find_esm(
&self,
specifier: &str,
) -> Option<&ExtensionFileSource> {
self
.get_esm_sources()?
.iter()
.find(|s| s.specifier == specifier)
}
}

// Provides a convenient builder pattern to declare Extensions
Expand Down
Loading

0 comments on commit 7a8bb3b

Please sign in to comment.