Align "fetch a single module script" behavior with whatwg/html#10327 [chromium/src : main]
Sunny Jerry (Gerrit)
Sunny Jerry added 1 comment![]( "Open in Gerrit")
Hi Hiroshige
This CL implements the behavior proposed in whatwg/html#10327 and passes the new WPT tests.
I know the spec change isn’t merged yet, the goal here is to help validate and move the proposal forward toward a better module loading model.
Would you mind taking a look or sharing thoughts on how we could proceed?
Thanks!
Related details
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
Hi Hiroshige
This CL implements the behavior proposed in whatwg/html#10327 and passes the new WPT tests.
I know the spec change isn’t merged yet, the goal here is to help validate and move the proposal forward toward a better module loading model.
Would you mind taking a look or sharing thoughts on how we could proceed?
Thanks!
Hi,
Thank you for working on this.
I expect detailed review (both in impl and especially in spec) will be somewhat delayed as we are still transitioning around scripting-related works.
Anyway sending some early response below.
The current CL looks like a little different from the current spec PR (e.g. introducing three-state `ModuleMap::Entry::State` which doesn't exists explicitly in the spec PR), but as far as I skimmed the code, maybe this is aiming to implement (and is mostly actually implementing) the spec PR relatively straightforwardly. Am I understanding correctly?
For validating implementability, the module loading and the re-fetching logic is spanning across Blink and V8 (which I'm not so familiar with) and possibly triggers crashes or unexpected behavior, and thus more test coverage would be helpful to confirm this should work robustly.
e.g. additional WPTs covering https://github.com/whatwg/html/pull/10327#issuecomment-2101783768 scenarios, at least Section "1.", i.e. how does this work when a submodule under a single `<script type=module src=...>` or `import()` failed to loading, especially the failed module is imported from multiple scripts?
(As far as I very briefly skimmed the CL, probably it should work but not sure)
Related details
- Sunny Jerry
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunny Jerry (Gerrit)
Sunny Jerry added 1 comment![]( "Open in Gerrit")
Hiroshige HayashizakiHi Hiroshige
This CL implements the behavior proposed in whatwg/html#10327 and passes the new WPT tests.
I know the spec change isn’t merged yet, the goal here is to help validate and move the proposal forward toward a better module loading model.
Would you mind taking a look or sharing thoughts on how we could proceed?
Thanks!
Hi,
Thank you for working on this.I expect detailed review (both in impl and especially in spec) will be somewhat delayed as we are still transitioning around scripting-related works.
Anyway sending some early response below.
The current CL looks like a little different from the current spec PR (e.g. introducing three-state `ModuleMap::Entry::State` which doesn't exists explicitly in the spec PR), but as far as I skimmed the code, maybe this is aiming to implement (and is mostly actually implementing) the spec PR relatively straightforwardly. Am I understanding correctly?
For validating implementability, the module loading and the re-fetching logic is spanning across Blink and V8 (which I'm not so familiar with) and possibly triggers crashes or unexpected behavior, and thus more test coverage would be helpful to confirm this should work robustly.
e.g. additional WPTs covering https://github.com/whatwg/html/pull/10327#issuecomment-2101783768 scenarios, at least Section "1.", i.e. how does this work when a submodule under a single `<script type=module src=...>` or `import()` failed to loading, especially the failed module is imported from multiple scripts?
(As far as I very briefly skimmed the CL, probably it should work but not sure)
Hey! Thanks for the quick review!
Please take your time, we may still have quite a few things to discuss, since the CL is about the web standard that still under discussion.
As you mentioned, I believe introducing a three-state enum can provide a safer and more intuitive way to implement the new behavior, while staying consistent with the spec behavior. In the previous implementation, since `map_` always held a pointer to the entry object, both synchronous and asynchronous fetch behaviors were safe and wouldn’t cause crashes. However, after removing the entry from `map_`, the order of some operations needs to be adjusted, and we also need to make sure that the entry object won’t be released too early once it’s no longer held by `map_`.
For these reasons, I chose to use a three-state enum instead of directly removing the entry, but I understand that aligning with the spec is important, so I’ve also implemented a version that removes the entry here:
https://chromium-review.googlesource.com/c/chromium/src/+/7085082
I’ve tried to check all the potential issues I could think of and added tests to ensure it doesn’t crash. If you perfer this CL, we can choose it instead of the current implementation
For the WPT tests you mentioned, I rewrote the repeated import tests for JSON, JavaScript, and CSS modules. Since unit tests provide better clarity and control, I covered these cases in the module map unit tests as well.
Related details
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |