Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSupport dynamic-import of module script #25439
Comments
|
Dynamic imports are apparently a significant piece of most webgpu example code as well as the testsuite. |
|
@kunalmohan https://github.com/jdm/servo/tree/dynamic-module is a branch that supports the most common cases for dynamic module imports (importing inside of a module script, and importing inside of a classic script). Hopefully this will be enough for you to run tests in the short term. |
|
I'm trying to help to rebase @jdm 's branch to align the refactored module script design. |
|
I basically fix the type checking but the dynamic module is broken now. Will try to figure out how to fix it :/ The branch is at https://github.com/CYBAI/servo/tree/dynamic-module |
|
I tested it with my latest webgpu code just now. It works fine with simple script like this. But while loading one of the live examples it panics with the following backtrace- I hope this helps in some way. |
|
Hmm, looks like there's code that runs an import from inside of a promise callback. I was hoping cases like that wouldn't be common; master...CYBAI:dynamic-modulediff-3fe97584f564214ec8e7ebbf91747e03R1350-R1359 is a short-term hack that grabs the currently executing script from the document, but that only works for JS executed directly from <script> elements. |
|
The solution is to attach important data to the private fields of JSScript objects, and I think ModuleOwner::Window will need to contain something else than an HTMLScriptElement, or else we need to tie JSScripts to HTMLScriptElements more directly somehow. The issue is that when we hit a dynamic import that's not already inside of an existing module, at the moment we need to pass a module owner to |
I see! Thanks for pointing that out! If so, I'd like to add one more branch to the
Yes! I think I can understand the issue now! We will need a proper owner type for dynamic module to handle it. I will look into it and update anything here! Also thanks @kunalmohan for helping to check it works in your repo! |
|
A different panic when running the live test-suite at http://gpuweb-cts-glsl.github.io/standalone/. The code rests in branch @jdm iirc, I reported a similar error while running my test example initially. You fixed it on that occasion. I wonder if this one's related. |
|
@kunalmohan if you could build with |
|
@jdm I tried a number of times, but couldn't reproduce the error on a dev build. On a release build with that feature, I got a similar backtrace as before expect with one additional line(above the 6th line of previous backtrace)- which probably isn't very helpful. Although, on a debug build a warning shows up- |
|
The output of the borrow failure when using |
|
I ran it on the latest master, facing a different error now- |
|
@kunalmohan @jdm sorry, I'm quite busy recently and don't have enough time to investigate the crash (#25439 (comment)) yet. But, I tried to fix some more tests for dynamic module yesterday but there are still some TIMEOUTs that I haven't investigated yet. I think I'm going to send a draft PR and bors will remind me to rebase if there's any conflicts. Also, I tried to run the http://gpuweb-cts-glsl.github.io/standalone/ page with latest master and it crashed on my local too so I think it might be related to bugs in existing module script instead of dynamic module. I will investigate it! Thanks! |
|
Draft PR at #27026 |
|
@kunalmohan @jdm Because the issue is basically not related to dynamic import, I filed #27029 and summarized at #27029 (comment). I'm going to send a PR for it. |
|
With Then, I saw we fail to compile the https://gpuweb-cts-glsl.github.io/out/webgpu/gpu_test.js script in Servo Will try to investigate what makes it fail. |
cool, so the issue is the web page is using optional chaining which causes a syntax error for Servo now.
We might need a smup to support the syntax. |
|
If possible, I would recommend submitting changes upstream to avoid using the latest bleeding edge JS features. |
|
We'll change this in the test-suite, so there won't be a need to support it yet. |
|
@jdm @CYBAI really appreciate you both helping me so much in my project! Thank you! I replaced the optional-chaining syntax and tested on my local system with the I also realised that we already have test-suite under Also I want to confirm one thing. The tests |
|
Yes, FAIL means the test failed some assertions, while ERROR means that a JS exception was raised. |
|
Interesting! The panic looks like it fails to parse the module specifier. servo/components/script/script_module.rs Lines 1094 to 1097 in c76d131
Hmmm, if it's a valid URL, it should be parse-able Also, if it works fine on Chrome and Firefox but not on Servo, maybe it's worth filing it as a new issue! Thanks! |
This comment was marked as outdated.
This comment was marked as outdated.
|
Live test suite has been updated after removing optional chaining operator (https://gpuweb.github.io/cts/standalone/). Good news is that the page loads successfully and the tests are visible and we can run and see the results as expected. |
After #23370 fixed, we can implement dynamic-import of module script.
Spec: https://html.spec.whatwg.org/multipage/#fetch-an-import()-module-script-graph
Gecko code for registering dynamic-import hook: https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/dom/script/ScriptLoader.cpp#960-1053