Better check for 'script is synchronous' #1036
Conversation
|
I can try adding a test as well when I get a chance. To properly recreate this I need to load larger scripts/css files through a URL, Is it safe to load an external package like (https://unpkg.com/three@0.94.0/build/three.js) to the test case? |
|
Hm, that would be quite flaky. Can't we do a while loop for 5 seconds to
mimic that behavior?
…On Mon, Oct 15, 2018 at 8:10 PM Preet ***@***.***> wrote:
I can try adding a test as well when I get a chance. To properly recreate
this I need to load larger scripts/css files through a URL, Is it safe to
load an external package like (
***@***.***/build/three.js) to the test case?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1036 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFrDb4IKvhjLH2ZIP0impko7wf1zKZubks5ulN2cgaJpZM4Xc5NR>
.
|
|
Just a while loop will not do it because the async script needs to run while browser is still waiting for a sync script to fetch. So a while loop followed by a script sourced to a javascript file may do the trick. |
|
I have added a test that consistently fails in Firefox without the fix, and passes with the fix. |
|
All tests are passing so that is fine. I will pull in the code to run tests on older browsers. |
| <script> | ||
| const start = Date.now(); | ||
| let diff = 0; | ||
| while (diff < 5000) { |
There was a problem hiding this comment.
Maybe add a clarifying comment here. Something like we mimic a long loading script to make sure the document readystate is loading when the loader executes.
There was a problem hiding this comment.
Added a detailed explanation in the comments
webcomponents-loader.js
Outdated
| // if readyState is 'loading', this script is synchronous | ||
| if (document.readyState === 'loading') { | ||
| // if readyState is 'loading' and this script is synchronous | ||
| if (document.readyState === 'loading' && (!document.currentScript.async)) { |
There was a problem hiding this comment.
Iirc, document.currentscript does not exist in type="module". Could you verify? (In that case, it would need to be document.currentScript && ...
|
The tests fail on Edge 17 and IE11. Are you able to debug these and potentially fix them or should I do so? |
|
I don't have access to a windows machine right now. :( |
|
New issue: webcomponents/polyfills#57 |
|
Sorry for the delay on this If this is an issue that is still important to you, could you remake this PR in the new monorepo? https://github.com/webcomponents/polyfills Thanks! |
Fixes https://github.com/webcomponents/webcomponentsjs/issues/1034