core(tags blocking first-paint): exclude script type=module#3676
core(tags blocking first-paint): exclude script type=module#3676patrickhulce merged 5 commits intoGoogleChrome:masterfrom
Conversation
patrickhulce
left a comment
There was a problem hiding this comment.
thanks for the contribution @thisizkp! would you mind adding a module entry to our smoketests?
lighthouse/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
Lines 35 to 36 in 76e51f3
| return new Promise((resolve, reject) => { | ||
| try { | ||
| const tagList = [...document.querySelectorAll('link, head script[src]')] | ||
| const tagList = [...document.querySelectorAll('link, head script[src], script[type]')] |
There was a problem hiding this comment.
to detect the inline modules, type="module" may not be always followed by the src attribute.
<script type="module">
import module from '../example.js';
module.test();
</script>There was a problem hiding this comment.
sure, but I mean we're trying to ignore modules so we'd be pulling them in just to ignore them
| !tag.hasAttribute('defer') && | ||
| !/^data:/.test(tag.src); | ||
| !/^data:/.test(tag.src) && | ||
| !(tag.getAttribute('type') === 'module'); |
There was a problem hiding this comment.
let's do tag.getAttribute('type') !== 'module'
There was a problem hiding this comment.
is this just for readability or anything I'm missing over?
I just wrote like this to make it consistent with the above checks
There was a problem hiding this comment.
well the others are like that only because there not using an operator, so yeah just easier to read :)
|
@patrickhulce did those minor fixes, not sure about adding module entry to smoke tests. Can you help me out with any pointers to docs, examples or any resources about doing that? |
|
The two lines I referenced are the best example. Basically you want to copy those two lines in that file, add lighthouse/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html Lines 35 to 36 in 76e51f3 |
|
Oh, I was thinking may be I should create a new file separately to test |
|
@patrickhulce After that mentioned change I did (locally), smoke tests are failing. Need some more time to fix that issue. Will try and get back to you If I get stuck. |
|
After adding this line Smoke tests are failing for |
|
You haven't pushed your changes so I'm not sure exactly what's failing 😄 but you've added a new URL so you'll probably need to update the (bump line 40 to mutation events is probably reporting the mutation events fired in dbw_tester.js so let's just change the URL to something that'll 404 like |
|
but if it's just a 404, the test will not give accurate results right? |
|
a blocking script that 404s is still a blocking script so it should work fine for our purposes (you can add |
|
blocked at this - If I bump up this number to 17 instead of 16, it works. But I'm curious about what is triggering it. |
Seems like chrome double requests 404 modules, so I went ahead and fixed up the smoketests since I've had you churn enough 😄 |
patrickhulce
left a comment
There was a problem hiding this comment.
thanks for helping us out here @thisizkp! 🎉 🎂
|
thank you for helping me out, got stuck so many times... but somehow finally merged 🎉 |
…rome#3676) * exclude script type='module' from render blocking warnings * pr fixes * add 404 sample module as script source * quick typo fix * fix smoketests
Fixes #3359