refactor(core): remove ext: modules from the module map#19040
Conversation
nayeemrmn
changed the title
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Sorry for a slow response @nayeemrmn, I've been busy preparing for v1.34 release. I really like the direction of this PR and I believe it cleans up a lot of concepts.
That said I need to take another look at core/modules.rs with fresh eyes before approving and landing it.
I also left some comments with questions and suggestions; but overall it's great.
mmastrac
left a comment
mmastrac
left a comment
There was a problem hiding this comment.
I love these changes and love to see so many special cases deleted.
I think there's only one nit and one change I'd like to see -- moving this new retain/rename operation so that it's attached to the extension macro itself and called after JS init, which helps decouple the code.
| /// modules as `node:` specifiers. | ||
| pub fn init_runtime_module_map(js_runtime: &mut JsRuntime) { | ||
| js_runtime.clear_module_map( | ||
| deno_node::SUPPORTED_BUILTIN_NODE_MODULES |
There was a problem hiding this comment.
I think this rename and retain operation callback should live in the extension definition macro (deno_core::extension!) itself.
There was a problem hiding this comment.
I think this rename and retain operation callback should live in the extension definition macro (deno_core::extension!) itself.
That was my first idea. The problem is we don't want this step to be taken when we're creating the intermediary RUNTIME_SNAPSHOT since the CLI extensions will want internal access. While thinking of how on earth I'd document or option-ise this very specific behaviour, it became clear to me that it's not a property of the extension but rather a bespoke operation called by the host only at the final step. So I added a procedural binding for it.
IMO It turns out that this solution is the less coupled one and is easier to delete if e.g. we figure out a better cross-crate JS extension interface.
There was a problem hiding this comment.
That makes sense to me. I think we can take another look at the runtime/realm setup process after this lands (especially if we can get modules working correctly in realms soon).
| impl NodeModulePolyfill { | ||
| pub fn module_name(&self) -> &'static str { | ||
| debug_assert!(self.specifier.starts_with("node:")); | ||
| &self.specifier[5..] |
There was a problem hiding this comment.
It was previously that #19040 (comment)
I think either way is fine because all of these should start with "node:"
| /// Clear the module map, meant to be used after initializing extensions. | ||
| /// Optionally pass a list of exceptions `(old_name, new_name)` representing | ||
| /// specifiers which will be renamed and preserved in the module map. | ||
| pub fn clear_module_map( |
There was a problem hiding this comment.
See runtime/worker comments - this should be called internally based on extension macro config
| // 2. Iterate through all extensions: | ||
| // a. If an extension has a `esm_entry_point`, execute it. | ||
|
|
||
| // TODO(nayeemrmn): Module maps should be per-realm. |
There was a problem hiding this comment.
Yeah, this code is unfortunately quite broken with realms.
|
After reviewing the code and feedback I'm good with this as-is. I'll approve this as-is. Great PR. |
5 participants
Rather than disallowing
ext:resolution, clear the module map after initializing extensions so extension modules are anonymized. This operation is explicitly called indeno_runtime. Re-injectnode:specifiers into the module map after doing this.Fixes #17717.