theKashey / used-styles

📝All the critical styles you've used to render a page.
MIT License
137 stars 9 forks source link

Is moving / removing styles mandatory? #20

Closed hnrchrdl closed 4 years ago

hnrchrdl commented 4 years ago

i am streaming my markup with react and interleave critical styles with createCriticalStyleStream. is moving / removing the styles really neccessry in this case? i don't have any issues with hydrating the client.

theKashey commented 4 years ago
  1. With "real" interleaving (possible only in stream rendering) moving should be mandatory before hydration to match generated HTML. However I never tested this moment, considering tested by for example emotion - some technical details were borrowed from their transformers. However there is another reason to move styles - you have to hoist them above "real" style definitions to let "real" styles override them if needed. This is more bound to how you write your styles.

  2. Removing is optional. However there is assumption, untested for modern browsers, that the less rules you have - the faster it runs.


How you write your styles

Let's imagine you have a critical styles written in BEM notation:

.myStyle {
 color: red;
}
.myStyle--version1 {
 color: blue;
}

And there is also one more modifier in the "real" styles

.myStyle--version2 {
 color: green;
}

If critical styles would be defined in HTML, and "real" style in the header - due to specificity rules .myStyle will always override .myStyle--version2, which is not what you probably want :)

hnrchrdl commented 4 years ago

Regarding the specificity, it makes sense. Otherwise it might introduce weird css issues. To be safe, I went with moving and removing the interleaved styles.

For anyone interested, I came up with this solution:

In the header template, I introduce a resolved and a count variable for any style tag:

<script>
  window.__styleTagsResolved__ = 0;
  window.__styleTagsCount__ = <%- styleTags.length %>;
</script>
<% styleTags.forEach(function(tag) { %>
  <%- tag %>
<% }); %>

StyleTags are defined like this (pattern for non-blocking style tags adopted from this post):

createNonBlockingStyleTag = (href: string) =>
    `<link rel="stylesheet" media="print" href="${href}" onload="this.onload=null; this.media='all'; window.__styleTagsResolved__++;">
<noscript><link rel="stylesheet" href="${href}"></noscript>`;

Notice that on resolving of any the the style tags, the number of resolved style tags is counted up.

On the client, before any hydration takes place, I do this to move the styles into the head first and then recursively check if the styles can be removed:

import { moveStyles, removeStyles } from 'used-styles/moveStyles';

moveStyles();
function checkRemoveStyles() {
    if (window.__styleTagsResolved__ < window.__styleTagsCount__) {
        setTimeout(checkRemoveStyles, 50);

        return;
    }
    removeStyles();
}
checkRemoveStyles();
theKashey commented 4 years ago

Speaking of non-blocking style tags - actually webpack mini-css-plugin and friends might inject "used" .css files on JS start, and consume some of network bandwidth in the moments when it's really needed for other resources. It might be a good idea to prevent such behaviour.

(like here and here)

hnrchrdl commented 4 years ago

interesting idea, thanks for sharing. in my case all script tags (which are preloaded) are inserted at the very bottom of the page and attributed async, so rendering should have already been done the browser (using the critical interleaved styles) by the time any JS is executed.

do you think it might still be an issue? what "other resources" are you thinking of? will images be affected? or other chunks of JS which are loaded at runtime?

theKashey commented 4 years ago

Other scripts might be affected, as long as styles are usually more "important" than everything else, while in this particular case could be delated.