Add native support for loading ES modules #1440
Conversation
| auto count = module_map_.count(filename); | ||
| if (count == 1) { | ||
| auto module = module_map_[filename].Get(isolate_); | ||
| resolve_module_.Reset(isolate_, module); |
There was a problem hiding this comment.
Does this mean that there are chances where we might fetch a source multiple times, but discard all the fetched source code and only using the first fetched version?
(As a side question, does V8 InstantiateModule guarantees that the same module would not be running in the same context twice?)
There was a problem hiding this comment.
I believe (tho I'm not sure, I haven't tested it) that the module_map_ will properly prevent that from happening. The resolve_module_.Reset() here is only to pass around a reference during resolution.
ry
force-pushed
the
esm_libdeno
branch
4 times, most recently
from
a066077 to
029ad3b
Compare
ry
force-pushed
the
esm_libdeno
branch
2 times, most recently
from
029ad3b to
dc956c5
Compare
For better greppability and conformance with other symbols in libdeno.rs
Introduces deno_execute_mod() for executing ES modules.
Seems to be necessary to get the previous commits to compile correctly.
| # Build incrementally. | ||
| - $(DENO_BUILD_PATH) | ||
| # TODO Incremental build disabled because there are some bugs. | ||
| #- $(DENO_BUILD_PATH) |
There was a problem hiding this comment.
It's about 11 minutes https://ci.appveyor.com/project/deno/deno/builds/21340582
| @@ -64,7 +64,7 @@ before_script: | |||
| script: | |||
| - ./tools/lint.py | |||
| - ./tools/test_format.py | |||
| - ./tools/build.py -C target/release -j2 | |||
There was a problem hiding this comment.
This PR is based on this #1444 (explanation there)
| v8::HandleScope handle_scope(isolate); | ||
| auto context = d->context_.Get(d->isolate_); | ||
| CHECK(!context.IsEmpty()); | ||
| return deno::ExecuteMod(context, js_filename, js_source) ? 1 : 0; |
There was a problem hiding this comment.
What's this return value? Success/error? In c++ code I would expect to use bool, or alternatively you can do common c way of 0 (ok) / -1 (fail).
There was a problem hiding this comment.
See docs in deno.h - this is to match the behavior of deno_execute 1 success / 0 fail.
(Yes I agree it should be the opposite, but let's do that in a separate PR.)
| // A callback to implement ES Module imports. User must call deno_resolve_ok() | ||
| // at most once during deno_resolve_cb. If deno_resolve_ok() is not called, the | ||
| // specifier is considered invalid and will issue an error in JS. The reason | ||
| // deno_resolve_cb does not return deno_module is to avoid unnecessary heap |
There was a problem hiding this comment.
does not return deno_module is to avoid unnecessary heap allocations
Did it seem very expensive? I doubt this is even in the vicinity of a hot path.
There was a problem hiding this comment.
Not so much about performance, but rather about keeping memory management simple. I don't want to have to think about allocating something in Rust and then passing ownership to libdeno
| EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "SnapshotBug()")); | ||
| deno_delete(d); | ||
| } | ||
|
|
||
| TEST(LibDenoTest, GlobalErrorHandling) { | ||
| Deno* d = deno_new(deno_config{0, snapshot, empty, nullptr}); | ||
| Deno* d = deno_new(deno_config{0, snapshot, empty, nullptr, nullptr}); |
There was a problem hiding this comment.
All these nullptr additions....
Maybe we should do:
DenoConfig config;
config.set_snapshot("foo");
deno_new(config);
There was a problem hiding this comment.
Yeah it's getting a little annoying to update these tests all the time. I'm open to it, but note that this is a C api, so the above wouldn't work.
The intention is eventually to move TS into its own isolate, so that the runtime can be faster and leaner. This patch sets up the libdeno bindings for this to happen. A subsequent PR will move the compiler to a different isolate.
Ref #975