jsg: dynamic imports #88
Conversation
Modules registered with addOnDemand method will be parsed and instantiated when all other imports failed.
mikea
requested review from
kentonv,
jasnell,
harrishancock,
jclee,
a-robinson,
bretthoerner and
byule
as code owners
|
@jasnell @harrishancock please take a look. |
| @@ -171,9 +171,9 @@ class ModuleRegistry { | |||
| ModuleInfo& operator=(ModuleInfo&&) = default; | |||
| }; | |||
|
|
|||
| virtual kj::Maybe<ModuleInfo&> resolve(const kj::Path& specifier) = 0; | |||
| virtual kj::Maybe<ModuleInfo&> resolve(v8::Isolate* isolate, const kj::Path& specifier) = 0; | |||
There was a problem hiding this comment.
Let's have resolve take a jsg::Lock& js instead of the isolate
There was a problem hiding this comment.
Unfortunately that would mean changing very long call chains, since they don't have lock wired through.
E.g.:
resolve <- evaluateSyntheticModuleCallback <- createSyntheticModule <- ModuleInfo with no lock in sight yet.
Let me know if you want to create locks before calling resolve or tackle passing jsg::Lock everywhere in modules.h
There was a problem hiding this comment.
I do eventually want to update modules.h to use jsg::Lock& throughout so if you don't want to do this now that's fine. That said, it's simple to grab the jsg::Lock for the current isolate without passing it through everywhere using auto& js jsg::Lock::from(isolate).
There was a problem hiding this comment.
I'll make separate PR with this cleanup today if you don't mind.
src/workerd/jsg/modules.h
Outdated
| return moduleInfo; | ||
| } | ||
| KJ_CASE_ONEOF(src, kj::ArrayPtr<const char>) { | ||
| info = ModuleInfo(isolate, specifier.basename()[0], src); |
There was a problem hiding this comment.
Just curious because it's not obvious...Why is this taking only the basename of the specifier and not the whole specifier?
There was a problem hiding this comment.
(to be clear, just saying that a comment here would be good :-) ...)
There was a problem hiding this comment.
Just curious because it's not obvious...Why is this taking only the basename of the specifier and not the whole specifier?
I actually don't know what's right: should module name contain full path or only basename? E.g. for import "node/path" what should the name be?
There was a problem hiding this comment.
To import node core modules it should be require('node:... ') where ... is the name of the core module. E.g. require('node:assert')
There was a problem hiding this comment.
I don't think this name is relate to how you import. This name goes to ScriptOrigin.
I don't know if it should be the same or not.
PS nit: only ESM import will be supported, not require.
There was a problem hiding this comment.
Why wouldn't require be supported? This should work for that also
There was a problem hiding this comment.
Why wouldn't require be supported?
Because then we'll have to compile two versions of the module? One for ESM and one for commonjs?
Or can ESM modules be imported safely using require too?
There was a problem hiding this comment.
The way we handle imports and require it should just work. Worth testing though
There was a problem hiding this comment.
(to be clear, it might not just work also)
src/workerd/jsg/modules.h
Outdated
| @@ -198,17 +198,25 @@ class ModuleRegistryImpl final: public ModuleRegistry { | |||
| entries.insert(Entry(specifier, kj::fwd<ModuleInfo>(info))); | |||
| } | |||
|
|
|||
| kj::Maybe<ModuleInfo&> resolve(const kj::Path& specifier) override { | |||
| void addOnDemand(const kj::Path& specifier, kj::ArrayPtr<const char> sourceCode) { | |||
There was a problem hiding this comment.
addOnDemand(...) seems like an odd name for this. I would expect something more descriptive like addBuiltinModule(...)
Also, where and when would this be called? And by whom? I know this is part of a large piece of work but expanding the comment here to provide some additional detail would help me as a reviewer evaluate the correctness of this.
There was a problem hiding this comment.
I have created a demo in EW PR 4479. Will rename the function shortly.
|
Method renamed, comments added. A change to replace Isolate with Lock in modules is underway. PTAL @jasnell |
I think I messed up my various git clients. Hold on please while I fix this. |
|
pushed the update. To recap:
ptal. @jasnell |
| @@ -276,19 +301,28 @@ class ModuleRegistryImpl final: public ModuleRegistry { | |||
| const Entry& keyForRow(const Entry& row) const { return row; } | |||
|
|
|||
| bool matches(const Entry& entry, const Entry& other) const { | |||
| return entry.info.hash == other.info.hash; | |||
| return hashCode(entry) == hashCode(other); | |||
There was a problem hiding this comment.
This looks like it has problems with hash collisions. It looks like previously hash codes were based strictly on V8 identity hashes which might plausibly be unique (I'm not sure), but now it's based on string hashes in some cases which definitely could collide. This function needs to change to actually perform a proper comparison.
| return moduleInfo.hash; | ||
| } | ||
| KJ_CASE_ONEOF(src, kj::ArrayPtr<const char>) { | ||
| return kj::hashCode(src); |
There was a problem hiding this comment.
I guess this is hashing the whole source code, which sounds kind of expensive. Maybe we could key off a pointer to the code instead? This always points to a static constant, right?
Modules registered with addOnDemand method will be parsed and instantiated when all other imports failed.