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

refactor: Use ES modules for internal runtime code #17648

Merged
merged 67 commits into from
Feb 7, 2023
Merged
Show file tree
Hide file tree
Changes from 56 commits
Commits
Show all changes
67 commits
Select commit Hold shift + click to select a range
925b649
work
crowlKats Feb 4, 2023
c6467d2
work
crowlKats Feb 4, 2023
3df04f9
deno_url translated to esm
bartlomieju Feb 4, 2023
b31144b
deno_webgpu
crowlKats Feb 4, 2023
3fe3908
deno_crypto translated to esm
bartlomieju Feb 4, 2023
83aa1ec
deno_web
crowlKats Feb 4, 2023
c76cf84
websocket
crowlKats Feb 4, 2023
22c2f3b
webstorage
crowlKats Feb 4, 2023
c28689f
fetch
crowlKats Feb 4, 2023
39e2125
cache
crowlKats Feb 4, 2023
c1aa1f6
40_testing & 90_deno_ns
crowlKats Feb 4, 2023
6617c40
40_testing & 90_deno_ns
crowlKats Feb 4, 2023
add5794
broadcast channel
crowlKats Feb 4, 2023
708fe3d
ffi
crowlKats Feb 4, 2023
33c0934
node
crowlKats Feb 4, 2023
dc7cbb9
net
crowlKats Feb 4, 2023
663a5d1
deno_http translated to esm
bartlomieju Feb 4, 2023
639b259
deno_flash translated to esm
bartlomieju Feb 4, 2023
f0e49cb
runtime
crowlKats Feb 4, 2023
6235a01
runtime/41_prompt.js translated to esm
bartlomieju Feb 4, 2023
10108b6
runtime/40_write_file.js translated to esm
bartlomieju Feb 4, 2023
68dae03
tty, write_file
bartlomieju Feb 4, 2023
07d36e6
make it compile
bartlomieju Feb 4, 2023
b491d3b
remove various globalThis.__bootstrap namespaces
bartlomieju Feb 4, 2023
a41a517
lint, remove debug logs
bartlomieju Feb 4, 2023
0da5a8f
cleanup files inclusion
crowlKats Feb 4, 2023
b39ac46
use default export for primordials
crowlKats Feb 4, 2023
3a714ab
fix
crowlKats Feb 4, 2023
568fcec
export ops obj from 01_core.js
crowlKats Feb 5, 2023
e440e48
internals
crowlKats Feb 5, 2023
49597de
internalsfix
crowlKats Feb 5, 2023
9ab7c00
remove all __bootstrap usages
crowlKats Feb 5, 2023
d86ec77
fix webgpu
crowlKats Feb 5, 2023
ce4acc0
cleanup domexception
crowlKats Feb 5, 2023
f1ca615
Merge branch 'main' into es_builtins
crowlKats Feb 5, 2023
ea4376b
update imports
crowlKats Feb 5, 2023
96e89f3
fmt
crowlKats Feb 5, 2023
f889210
clean up
crowlKats Feb 5, 2023
c40c26d
lint
crowlKats Feb 5, 2023
5d36380
fix
crowlKats Feb 5, 2023
35afbff
fix
crowlKats Feb 5, 2023
0d8db69
fix
crowlKats Feb 5, 2023
8429461
fix some tests
crowlKats Feb 5, 2023
de99562
add debug logs
crowlKats Feb 5, 2023
7674d0e
fmt
crowlKats Feb 5, 2023
043f96d
always use InternalModuleLoader
crowlKats Feb 6, 2023
37eee91
Merge branch 'main' into es_builtins
crowlKats Feb 6, 2023
c8a3d70
fix & add test
crowlKats Feb 6, 2023
5b79025
fmt
crowlKats Feb 6, 2023
fe5124f
fix one panic
bartlomieju Feb 6, 2023
6437dee
fallback to regular loader
bartlomieju Feb 6, 2023
10ce27a
fix tests for modules
bartlomieju Feb 6, 2023
14ace36
revert core
crowlKats Feb 6, 2023
de0a21d
fix
crowlKats Feb 6, 2023
c1e9b5d
fix tests
crowlKats Feb 6, 2023
eee46d8
cleanup unwraps
crowlKats Feb 6, 2023
4078801
fix interna.d.ts files
crowlKats Feb 6, 2023
2287ad5
address comments
crowlKats Feb 6, 2023
6be1fd3
cleanup InternalModuleLoader usage
crowlKats Feb 6, 2023
8123feb
fix test
crowlKats Feb 7, 2023
23bf8de
fix typings
crowlKats Feb 7, 2023
8818a91
reject dynamic imports for internal modules
bartlomieju Feb 7, 2023
6d091cb
Merge branch 'main' into es_builtins
bartlomieju Feb 7, 2023
665c5c7
lint
crowlKats Feb 7, 2023
f823a16
fix std
crowlKats Feb 7, 2023
8d622e8
fix streams
crowlKats Feb 7, 2023
769273f
add InternalModuleLoader test
crowlKats Feb 7, 2023
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
3 changes: 3 additions & 0 deletions bench_util/js_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ use crate::profiling::is_profiling;
pub fn create_js_runtime(setup: impl FnOnce() -> Vec<Extension>) -> JsRuntime {
JsRuntime::new(RuntimeOptions {
extensions_with_js: setup(),
module_loader: Some(std::rc::Rc::new(
deno_core::InternalModuleLoader::new(None),
)),
..Default::default()
})
}
Expand Down
14 changes: 8 additions & 6 deletions cli/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ mod ts {
.build()],
extensions_with_js: vec![],
additional_files: files,
additional_esm_files: vec![],
compression_cb: Some(Box::new(|vec, snapshot_slice| {
vec.extend_from_slice(
&zstd::bulk::compress(snapshot_slice, 22)
Expand Down Expand Up @@ -306,7 +307,7 @@ mod ts {
}
}

fn create_cli_snapshot(snapshot_path: PathBuf, files: Vec<PathBuf>) {
fn create_cli_snapshot(snapshot_path: PathBuf, esm_files: Vec<PathBuf>) {
let extensions: Vec<Extension> = vec![
deno_webidl::init(),
deno_console::init(),
Expand Down Expand Up @@ -343,7 +344,8 @@ fn create_cli_snapshot(snapshot_path: PathBuf, files: Vec<PathBuf>) {
startup_snapshot: Some(deno_runtime::js::deno_isolate_init()),
extensions,
extensions_with_js: vec![],
additional_files: files,
additional_files: vec![],
additional_esm_files: esm_files,
compression_cb: Some(Box::new(|vec, snapshot_slice| {
lzzzz::lz4_hc::compress_to_vec(
snapshot_slice,
Expand Down Expand Up @@ -448,13 +450,13 @@ fn main() {
let o = PathBuf::from(env::var_os("OUT_DIR").unwrap());

let compiler_snapshot_path = o.join("COMPILER_SNAPSHOT.bin");
let js_files = get_js_files(env!("CARGO_MANIFEST_DIR"), "tsc");
let js_files = get_js_files(env!("CARGO_MANIFEST_DIR"), "tsc", None);
ts::create_compiler_snapshot(compiler_snapshot_path, js_files, &c);

let cli_snapshot_path = o.join("CLI_SNAPSHOT.bin");
let mut js_files = get_js_files(env!("CARGO_MANIFEST_DIR"), "js");
js_files.push(deno_runtime::js::get_99_main());
create_cli_snapshot(cli_snapshot_path, js_files);
let mut esm_files = get_js_files(env!("CARGO_MANIFEST_DIR"), "js", None);
esm_files.push(deno_runtime::js::get_99_main());
create_cli_snapshot(cli_snapshot_path, esm_files);

#[cfg(target_os = "windows")]
{
Expand Down
2,509 changes: 1,253 additions & 1,256 deletions cli/js/40_testing.js

Large diffs are not rendered by default.

12 changes: 12 additions & 0 deletions cli/tests/integration/run_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3844,3 +3844,15 @@ itest!(node_prefix_missing {
envs: env_vars_for_npm_tests_no_sync_download(),
exit_code: 1,
});

itest!(internal_import {
args: "run run/internal_import.ts",
output: "run/internal_import.ts.out",
exit_code: 1,
});

itest!(internal_dynamic_import {
args: "run run/internal_dynamic_import.ts",
output: "run/internal_dynamic_import.ts.out",
exit_code: 1,
});
1 change: 1 addition & 0 deletions cli/tests/testdata/run/internal_dynamic_import.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
await import("internal:runtime/js/01_build.js");
4 changes: 4 additions & 0 deletions cli/tests/testdata/run/internal_dynamic_import.ts.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
error: Uncaught (in promise) TypeError: Cannot load internal module from external code
Copy link
Member

Choose a reason for hiding this comment

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

How does CLI get to the point of this error? This error is thrown by the internal module loader, not the cli loader. What is up with that?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point @crowlKats, we shouldn't get an error from the CLI loader, not the internal loader.

Copy link
Member

Choose a reason for hiding this comment

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

Why was this resolved? AFAICT, the error is still coming from the internal loader. Why is the internal loader used at runtime in CLI?

Copy link
Member

Choose a reason for hiding this comment

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

@lucacasonato it's not coming from internal loader - I disabled dynamic imports of internal modules completely in core/bindings.rs - so we're not even hitting module system, nor any of the loaders.

Copy link
Member

Choose a reason for hiding this comment

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

Can we do the same for resolution in static loading?

Copy link
Member Author

Choose a reason for hiding this comment

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

static loading is erroring before currently, because the FileFetcher is erroring with invalid scheme

Copy link
Member

Choose a reason for hiding this comment

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

I think we could do that somewhere in core/modules.rs - essentially you're asking to hard disable loading of internal modules in cases where we are running from a snapshot?

Copy link
Member Author

Choose a reason for hiding this comment

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

essentially you're asking to hard disable loading of internal modules in cases where we are running from a snapshot?

we already are doing that though, no? the InternalModuleLoader isnt used by cli; its not used if one is loading a snapshot but not creating one

Copy link
Member

Choose a reason for hiding this comment

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

Please do the enforcement at core level in a follow up as Bartek described. CLI is not the only loader implementation. Not all loaders enforce a given scheme during resolution.

await import("internal:runtime/js/01_build.js");
^
at async [WILDCARD]
1 change: 1 addition & 0 deletions cli/tests/testdata/run/internal_import.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import "internal:runtime/js/01_build.js";
8 changes: 8 additions & 0 deletions cli/tests/testdata/run/internal_import.ts.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: Unsupported scheme "internal" for module "internal:runtime/js/01_build.js". Supported schemes: [
"data",
"blob",
"file",
"http",
"https",
]
at [WILDCARD]
7 changes: 0 additions & 7 deletions cli/tests/unit/globals_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,6 @@ Deno.test(function globalThisExists() {
assert(globalThis != null);
});

Deno.test(function noInternalGlobals() {
// globalThis.__bootstrap should not be there.
for (const key of Object.keys(globalThis)) {
assert(!key.startsWith("_"));
}
});

crowlKats marked this conversation as resolved.
Show resolved Hide resolved
Deno.test(function windowExists() {
assert(window != null);
});
Expand Down
2 changes: 2 additions & 0 deletions core/01_core.js
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,8 @@
});

ObjectAssign(globalThis.__bootstrap, { core });
const internals = {};
ObjectAssign(globalThis.__bootstrap, { internals });
ObjectAssign(globalThis.Deno, { core });

// Direct bindings on `globalThis`
Expand Down
16 changes: 16 additions & 0 deletions core/extensions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ impl OpDecl {
#[derive(Default)]
pub struct Extension {
js_files: Option<Vec<SourcePair>>,
esm_files: Option<Vec<SourcePair>>,
ops: Option<Vec<OpDecl>>,
opstate_fn: Option<Box<OpStateFn>>,
middleware_fn: Option<Box<OpMiddlewareFn>>,
Expand Down Expand Up @@ -88,6 +89,13 @@ impl Extension {
}
}

pub fn init_esm(&self) -> &[SourcePair] {
crowlKats marked this conversation as resolved.
Show resolved Hide resolved
match &self.esm_files {
Some(files) => files,
None => &[],
}
}

/// Called at JsRuntime startup to initialize ops in the isolate.
pub fn init_ops(&mut self) -> Option<Vec<OpDecl>> {
// TODO(@AaronO): maybe make op registration idempotent
Expand Down Expand Up @@ -145,6 +153,7 @@ impl Extension {
#[derive(Default)]
pub struct ExtensionBuilder {
js: Vec<SourcePair>,
esm: Vec<SourcePair>,
ops: Vec<OpDecl>,
state: Option<Box<OpStateFn>>,
middleware: Option<Box<OpMiddlewareFn>>,
Expand All @@ -164,6 +173,11 @@ impl ExtensionBuilder {
self
}

pub fn esm(&mut self, js_files: Vec<SourcePair>) -> &mut Self {
self.esm.extend(js_files);
self
}

pub fn ops(&mut self, ops: Vec<OpDecl>) -> &mut Self {
self.ops.extend(ops);
self
Expand Down Expand Up @@ -195,10 +209,12 @@ impl ExtensionBuilder {

pub fn build(&mut self) -> Extension {
let js_files = Some(std::mem::take(&mut self.js));
let esm_files = Some(std::mem::take(&mut self.esm));
let ops = Some(std::mem::take(&mut self.ops));
let deps = Some(std::mem::take(&mut self.deps));
Extension {
js_files,
esm_files,
ops,
opstate_fn: self.state.take(),
middleware_fn: self.middleware.take(),
Expand Down
Loading