refactor: module loading in EsIsolate #3615
Conversation
|
Summary of changes:
All in all this PR moves us forward greatly in terms of having Isolate living on a single thread. CC @ry |
bartlomieju
changed the title
|
Great work - these are the sort of refactors that really help us move forward
Awesome!
What does this mean? |
|
|
||
| mods_: HashMap<ModuleId, ModuleInfo>, | ||
| loader: Arc<Box<dyn Loader + Unpin>>, | ||
| pub modules: Modules, |
There was a problem hiding this comment.
I see Modules is basically a wrapper around the previous HashMap ?
| ) -> Result<(), ErrBox> { | ||
| let ctx = ResolveContext { resolve_fn }; | ||
| self.mod_instantiate2(ctx, id); | ||
| fn mod_instantiate(&mut self, id: ModuleId) -> Result<(), ErrBox> { |
There was a problem hiding this comment.
Not pub - use Isolate::load_module 🎉
| by_name: ModuleNameMap, | ||
| pub(crate) specifier_cache: HashMap<(String, String), ModuleSpecifier>, | ||
| } |
There was a problem hiding this comment.
I glossed over the creation of this struct in your previous PR - but LGTM 👍
There was a problem hiding this comment.
specifier_cache is used to simplify module resolve callback - it is no longer needed to create closures (which were wrong anyway...) and pass pointers to them when you instantiate module. Everything is now encapsulated in Isolate::module_resolve_cb method
| // asynchronous module loading. | ||
| // This module provides higher level implementation of Isolate that | ||
| // supports asynchronous loading and executution of ES Modules. | ||
| // The isolate.rs should never depend on this module. |
There was a problem hiding this comment.
Some triple slash comments on EsIsolate would be good for docs.rs
There was a problem hiding this comment.
We'll add in follow-up PR 👍
No description provided.