Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: consider media query when filtering used selectors #22

Merged
merged 1 commit into from
Jun 7, 2020

Conversation

hnrchrdl
Copy link
Contributor

@hnrchrdl hnrchrdl commented Jun 7, 2020

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
Copy link
Owner

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 theKashey merged commit 925e932 into theKashey:master Jun 7, 2020
@theKashey
Copy link
Owner

theKashey commented Jun 7, 2020

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 hnrchrdl deleted the bug/usedselectors branch June 7, 2020 22:27
@hnrchrdl hnrchrdl restored the bug/usedselectors branch June 7, 2020 22:29
@hnrchrdl
Copy link
Contributor Author

hnrchrdl commented Jun 7, 2020

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

@theKashey
Copy link
Owner

v2.1.2 released

@hnrchrdl
Copy link
Contributor Author

hnrchrdl commented Jun 8, 2020

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
Copy link
Owner

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
Copy link
Contributor Author

hnrchrdl commented Jun 8, 2020

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
Copy link
Contributor Author

hnrchrdl commented Jun 8, 2020

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
Copy link
Owner

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
Copy link
Owner

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

@hnrchrdl
Copy link
Contributor Author

hnrchrdl commented Jun 9, 2020

2.1.3 works like a charm. thanks!

@theKashey
Copy link
Owner

🎉 🎉 🎉 🎉 From the first try!

@hnrchrdl hnrchrdl deleted the bug/usedselectors branch June 9, 2020 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

media query support
2 participants