theKashey / used-styles

πŸ“All the critical styles you've used to render a page.
MIT License
138 stars 10 forks source link

fix: consider media query when filtering used selectors #22

Closed hnrchrdl closed 4 years ago

hnrchrdl commented 4 years ago

this fixes #21.

not sure if this is the best way to fix it. also adding tests for this was not super straight forward to me.

theKashey commented 4 years ago

Yep, now I understood the problem. As well as it's bound only to stream version of getCriticalStyles.

Your change would work now, however I'll change it a bit later, as long as it's a good way to solve #10 - just filter them out.

theKashey commented 4 years ago

Look like using string, any string, to filter repetitive rules is not a good idea. We should use blocks as-is. Thank you for discovering such serious design flaw.

hnrchrdl commented 4 years ago

thanks, looking forward to a release with a proper fix. let me know if i can support you in any way.

theKashey commented 4 years ago

v2.1.2 released

hnrchrdl commented 4 years ago

thanks, that was quick. the original problem is now fixed.

however when I inspect the page source, i can see a lot of duplicated css now.

theKashey commented 4 years ago

Oh, no way. Could you give a few examples of such duplication so I'll try to understand what's happening.

For example they could come from different files, containing the same rules by any reason. I was thinking about using not WeakSets to track used selectors, but generate a hash from every selector (on the setup stage), and using those hashes.

hnrchrdl commented 4 years ago

actually coming from the same file, i had a complete set of css rules added to the page more than 10 times.

not sure about the implementation details, but a StyleSelector block is carrying also a declaration property as a number, maybe that is the reason for blocks not being weakly identical?

i don't have a small reproducible example at hand right now, but i will look into my case and try to figure out what is going on.

a hash would be another option if it is not too costly at runtime.

hnrchrdl commented 4 years ago

ok, you are right, the css rules come indeed from different files. webpack somehow spreads the same rules over a lot of different css files.

solution: for any given route, i know which css files are being used. so i can modify the style definition passed to createCriticalStyleStream to only include those files per route. that should solve my issue.

once again, thanks for your help and all the work you put into this!

theKashey commented 4 years ago

webpack somehow spreads the same rules over a lot of different css files.

That's a normal behavior for sass include - it just adds rule into the file, multiplicating by the number or your chunks. Resent versions support @use to prevent situations like this.

However, in our case only per-selector "hashing" could save the day.

theKashey commented 4 years ago

Try 2.1.3 - it uses content hashes instead of weak refs and should keep only one version of a selector.

hnrchrdl commented 4 years ago

2.1.3 works like a charm. thanks!

theKashey commented 4 years ago

πŸŽ‰ πŸŽ‰ πŸŽ‰ πŸŽ‰ From the first try!