Open espoal opened 2 years ago
Hey @espoal, thanks for creating this issue. Can you provide a "not working" example? I can't spot any differences in the provided links
Hey @bezoerb thank you for your quick reply.
Sorry I was playing with the repo and inadvertently deleted the route in firebase. Try again, and try ctrl+shift+r
, because I'm also testing a service worker and I messed up a bit the caching.
And just to make it even more clear: the calendar SVG in the datepicker overlaps the date after using critical.
If you want to look at the output is here, and if you want to look how I generated it it's here.
If you want to try locally just fix the hard paths (/home/mamluk/....
) in libs/critical/index.mjs
, I was too lazy to use dynamic paths in a test :D
I tried to debug on my local machine but i'm missing the firebase token ;)
Nevertheless, i think the problem is caused by the extract: true
setting.
This leads to the following:
<style>
...
.pl-9 {
padding-left: 2.25rem;
}
...
</style>
<link rel="stylesheet" href="/uncritical.css" media="all" onload="this.media='all'">
uncritical.css contains
[type=text] {
padding-left: 0.75rem;
}
but the pl-9
class is removed because of extract which then leads to your css beeing in the wrong order. Classes and attribute selectors have the same specificity so the latter wins and the input gets a padding-left value of 0.75rem
.
I think it's currently not possible to place the <link rel="stylesheet" href="/uncritical.css" media="all" onload="this.media='all'">
before the <style>
tag but doing so would most likely result in other styles being messed up.
You can try playing around with the different inlining strategies provided by inline-critical
. You can change the strategy by passing a config object for inline instead of true
|false
but i guess the results will stay wrong.
critical.generate({
...
inline: {
strategy: 'default'
},
...
For now i would suggest removing the extract
option and trying the 'default'
strategy even if i'm wondering why the [type=text]
styles are missing in the critical css. In that case they should be removed from the uncritical.css, too.
Maybe @pocketjoso has more insights on this :)
@espoal: Any updates on this?
@bezoerb sorry I was stuck with some backend topics, I will try to answer before monday.
@bezoerb I'm sorry for the super late reply, and thank you very much for keeping this alive, I really appreciate it.
Unfortunately time is scarce, and my attention had to be diverted on some backend topics.
About the firebase issue: in theory no token is needed. Have you installed firebase emulator? what errors exactly should show? My repo should be an open source tutorial so my coworkers (and others) can follow it for new projects. If any token is asked for, then there must be a bug somewhere.
Unfortunately I tried all your tips without much success. In the end I decided that service workers are good enough: even though they are much much more complex, they provide similar benefits in term of loading. If we could make this work though, that would be amazing and I could backport it in my coworkers projects.
After using critical, there's a mismatch in the computed styles:
Without critical, and with critical
If you patiently inspect the styles, you see there's a mismatch in the computed styles, especially
border-radius
I tried to fix the issue by using
forceInclude
to no avail. It seems that when splitting the styles, theuncritical.css
gets precedence over the inlined styles.Btw: Amazing project. I took a commercial dashboard, whose load time was around 10 second, and I brought it to 2 seconds with a lot of work (ssr, lazy load, minification...) and then to 0.8 seconds with critical. This npm had by far the most impact, with the least amount of work, it should be advertised everywhere.