| 18:41 | <guybedford> | Further to the discussion last meeting about source phase architecture in v8 being tough to integrate |
| 18:41 | <guybedford> | both Deno and Cloudflare now resolve dependencies of source phase moudles, effectively in violation of the spec, just because that's the natural implementation path .... :( |
| 18:41 | <guybedford> | https://github.com/denoland/deno/issues/31240 |
| 19:23 | <Chengzhong Wu> | Deno didn't implement V8's source phase callbacks. Would you mind elaborating why this is a V8 issue? |
| 19:24 | <Chengzhong Wu> | In other words, Deno didn't implement V8's source phase support at all. |
| 19:28 | <guybedford> | Yeah I see that's the root cause here. Would be nice if implementers didn't have to do anything to get source phase though... |
| 19:29 | <Chengzhong Wu> | I'm not sure it will help in Deno's case |
| 19:29 | <Chengzhong Wu> | Deno is literally resolving before even reaching to V8 |
| 19:31 | <guybedford> | yeah, I see that, and that makes sense I suppose. Ideally implementations would just need to opt-out of their prefetching for source phase, and then all phases and imports work out after that. |
| 19:32 | <Chengzhong Wu> | If an embedder did not implement the source phase support, V8 does not enable the feature by default (at the moment, this is still hidden behind the flag --js-source-phase-imports) |
| 19:33 | <Chengzhong Wu> | I can see that this could be potentially improved by asking V8 taking over the resolution step, so that V8 can stop resolving source import dependencies |
| 19:34 | <Chengzhong Wu> | However, I'd doubt the complexity of it because resolution can be either sync (node.js) or async (web & node.js). |
| 19:35 | <guybedford> | resolution is always now synchronous on both the web and node |
| 19:35 | <Chengzhong Wu> | node still allows async resolution |
| 19:36 | <guybedford> | is it not fully deprecated yet? |
| 19:36 | <Chengzhong Wu> | it's.. not even under the discussion of deprecation AFAICT |
| 19:38 | <guybedford> | I guess there's two main architectures that might work for V8 - HostResolveImportedModuleSource as the new primitive that obtains sources first as a separate object (that can have compile cache etc) |
| 19:38 | <guybedford> | or we stick with HostResolveImportedModuleV2 or HostResolveImportedModuleWithPhase that takes a phase argument as a hint, then returns a module without resolving dependencies |
| 19:39 | <guybedford> | Yeah, so a new v2 / or we extend it to apply to all modules |
| 19:40 | <Chengzhong Wu> | not sure if we are referring to the same thing. Are you referring to HostImportModuleWithPhaseDynamicallyCallback? |
| 19:40 | <guybedford> | yes, as either being replaced or extended |
| 19:41 | <Chengzhong Wu> | HostImportModuleWithPhaseDynamicallyCallback is only invoked for import.source() calls, it already takes an argument of ModuleImportPhase |
| 19:41 | <guybedford> | I understand that and I've implemented that in both Node.js and Cloudflare |
| 19:42 | <guybedford> | what I'm talking about is how we extend this to work for JS modules and sources more generally, while avoiding implementer complexity, as implementer feedback |
| 19:44 | <joyee> | It no longer does at least on the main thread (only allows it on the async loader hook thread when async loader hooks are registered) |
| 19:44 | <Chengzhong Wu> | Putting Deno's issue aside, it's not easy for me to understand what's the exact issue you are trying to improve. I assume you are trying to push V8 taking over the implementation of resolution steps, and ask host to resolve for each referrer, specifier, attributes pairs? |
| 19:44 | <guybedford> | no, just to separate source phase from instance phase in the V8 pipeline |
| 19:44 | <Chengzhong Wu> | but... it is still allowed, right? |
| 19:44 | <guybedford> | sources as first-class representations in v8 |
| 19:45 | <guybedford> | i.e. both for WebAssembly Module Record, and for Source Text Module Record |
| 19:45 | <Chengzhong Wu> | i.e. we still need to support async resolution, not being able to remove the support entirely |
| 19:47 | <joyee> | We technically could, it's just going to be a breaking change for a fairly niche use case in an experimental feature (when you expect an async loader to affect the resolution of subsequent loader on the loader thread - say you register a typescript loader, and you want to load a zip loader written in typescript, that sort of thing) |
| 19:47 | <Chengzhong Wu> | I think the current v8::Module maps to the Module Record in the spec text. Both are essentially representation of module instance record |
| 19:47 | <Chengzhong Wu> | https://github.com/tc39/proposal-esm-phase-imports/issues/53 |
| 19:48 | <guybedford> | right, but in the spec [[ModuleSource]] is a field on the module record |
| 19:48 | <guybedford> | so we need to use the module as the handle for the module source, or separate the module source wrapper into its own thing |
| 19:48 | <guybedford> | as I say two main paths |
| 19:49 | <Chengzhong Wu> | V8 did not expose internal AbstractModuleSource to the API. It could be potentially exposed as a first class representation. But it'd still be great to distinguish it in the spec as a first step |
| 19:49 | <guybedford> | having v8::WasmModule will help I think too |
| 19:52 | <Chengzhong Wu> | Not speaking on behalf of V8 team. v8::WasmModuleObject is already exposed as a public API. V8 could expose v8::ModuleSource and make v8::WasmModuleObject a subclass of it |
| 19:54 | <guybedford> | The source phase as fully compiled can work for that, would require the JS representation to also have a fully compiled analog then |
| 19:55 | <guybedford> | would be nice if it can be associated with arbitrary cache data though |
| 19:56 | <Chengzhong Wu> | The C++ public API is not essentially 1:1 mapping with JS representation actually. When --js-source-phase-imports is set, JS WebAssembly.Module is a subclass of AbstractModuleSource, regardless of the v8::WasmModuleObject C++ hierarchy. |
| 19:56 | <joyee> | That pattern of using loader might already be somewhat problematic, regardless of the async requirement - typescript/zip don't even need to be async anyway (suppose loader A requires B to be loaded before A, and loader C requires itself to be loaded after A but before B, or you have multiple versions of one of these loaders...things easily get out of hand) |
| 19:56 | <guybedford> | this is separate from WebAssembly Module Record though |
| 19:58 | <Chengzhong Wu> | Addressing https://github.com/tc39/proposal-esm-phase-imports/issues/53 could help engines like V8 to define what state should come with a source record, and what state should come with an instance record, being spec-complaint |
| 20:00 | <guybedford> | How would it help to see that addressed? A more detailed explainer document around semantics and state that is carried? |
| 20:01 | <Chengzhong Wu> | Like, as you mentioned, making Module Source Record a first class record in ecma262 could be a first step |
| 20:01 | <Chengzhong Wu> | right now, the spec says "ModuleSource instances have a [[SourceTextModuleRecord]] internal slot." which could be confusing |
| 20:03 | <guybedford> | here's the diagram |
| 20:04 | <guybedford> | yes we need to decide if we want to split out module source, but from a spec perpsective we already discussed and determined this isn't necessary and because it affects three separate specifications that is a huge refactoring |
| 20:04 | <guybedford> | so I think addressing the question separately for the spec and v8 makes sense |
| 20:04 | <guybedford> | we're talking about v8 here - and need to answer that question now |
| 20:05 | <Chengzhong Wu> | I was saying that making this clearer in spec could help engines implement it correctly |
| 20:05 | <Chengzhong Wu> | and engines could expose 1:1 API mapping to the spec |
| 20:05 | <guybedford> | i appreciate that, but this might be one of those scenarios where spec complexity and implementation complexity require separate modularity approaches |
| 20:07 | <guybedford> | Allen always said that standards are not implementation algorithms but instead formal definitions that full cover the semantics |
| 20:08 | <Chengzhong Wu> | without a source of truth for the source / instance record hierarchy, i think defining the hierarchy in an engine by engine basis does not help in the long term. There are already name ambiguity in both spec and implementation that sometime "module" refers to a module instance, and sometime "module" refers to a source representation |
| 20:09 | <guybedford> | I think we iron this out with good tests and that is my focus here |
| 20:09 | <guybedford> | covering the cases of module serialization and importability across contexts |
| 20:10 | <Chengzhong Wu> | if not refactoring ecma262, an (official) explainer as a source of truth could also help |
| 20:13 | <guybedford> | Sure, that's good feedback, I'll put some thought to along with the test262 work. Appreciated spec on its own is not enough to clearly explain the model. |
| 20:14 | <guybedford> | (and especially if we have determined spec structure is not necessarily implementation structure, although tbd as well!) |