Skip to content

Splitting out Sapper's CSS handling #29

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

Closed
wants to merge 2 commits into from

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Sep 3, 2020

@Rich-Harris
Copy link
Member

The main reason for the current sapper-internal song and dance — which I completely agree is egregiously complicated and prevents useful things like additional transformations — is that we're able to extract styles into .css files (while also allowing Sapper to load those files before running the JS that depends on them).

I worry that a change that puts CSS in JS strings instead will harm performance, partly because of the double-loading effect you mention in the RFC, but also because increasing JS payloads is inherently bad (rather than being able to fetch a .css file and a .js module for a new route in parallel, we're fetching a single .js module that is larger than both combined, and increasing the cost of parsing said module).

@benmccann
Copy link
Member Author

I think it's pretty complicated in practice. While this would increase the JS payload it also reduces the number of requests and improves compression. E.g. things like the CSS class names will be shared between the JS and CSS. Also, compression works better on larger files than smaller ones.

I tried to research this a bit. It doesn't seem like there's a clear winner one way or the other. The benchmarks I found showed concatenation to be an improvement even with HTTP/2, but probably aren't representative of a typical Sapper app.

https://medium.com/@asyncmax/the-right-way-to-bundle-your-assets-for-faster-sites-over-http-2-437c37efe3ff:

Even in HTTP/2 environment, any level of concatenation showed a significant improvement compared to non-concatenation.
Differences [sic] were negligible.

https://medium.com/@jacobtan/understanding-http-2-and-its-caveats-1e8200519c4c:

Unlike domain sharding, asset concatenation might not be an anti-pattern as it is perceived to be in HTTP/2. While it is generally a good idea to keep your files small, there might be other factors that make serving multiple files over HTTP/2 slower than it should be.
For Khan Academy, they found out that serving just under 300 javascript files to a single page led to a degradation in performance, due to the inefficiencies in compressing smaller files, as opposed to compressing a much bigger file.

https://spinupwp.com/performance-best-practices-http2/:

Although HTTP/2 requests are cheap you may see improved performance by concatenating modules logically

@Rich-Harris
Copy link
Member

Agree that it's essentially an empirical question. I think the research around what happens when you serve 100+ small modules over HTTP/2 probably isn't hugely relevant to our case, since we're really talking about n JS modules (where n is typically 1-3 for a given navigation) vs n JS modules plus 1 CSS file — I'd naively expect it to take less time to fetch two 5kb resources than one 10kb resource, though I could be wildly wrong about that.

The point about compression is well made though. Maybe we should try this approach on whatever reasonably-sized Sapper apps we have to hand (e.g. svelte.dev) and try and figure out a concrete answer to the question. Might be harder to pin down the effects of having more JS to parse, though I guess these are probably small (my understanding is that JS engines are quite fast at parsing strings).

Something else that occurred to me is whether or not coarse-grained CSS chunks (the status quo) have better performance characteristics than adding a <style> element per component class (what we get with add_css). I would expect the latter to be slightly more taxing as the browser presumably has to update the CSSOM multiple times (though maybe if they're all added in the same tick it's a non-issue?). I don't know off the top of my head if rollup-plugin-postcss is able to chunk CSS together before injecting it with style-inject, but if so then perhaps that sidesteps the question.

@benmccann
Copy link
Member Author

benmccann commented Sep 5, 2020

I did some benchmarking, which I wrote up in my first blog post in three years, and it looks like it no longer matters whether you load CSS first of JS first. This doesn't answer all the other questions, but at least means that's probably not a blocker here.

@benmccann
Copy link
Member Author

benmccann commented Sep 8, 2020

For svelte.dev, today there's a main.css included on every page that is 31.3kB raw and 8.1kB compressed with Brotli. With the PR that turns into a client.css that's 21kB raw and 2.7kB compressed with Brotli. A good part of this reduction in size is better being able to handle dynamic imports.

However, I have noticed a bit of a flash of unstyled content (FOUC). In webpack this is typically solved by:

  • Using singletonStyleTag to reduce but not eliminate the FOUC
  • Using a plugin like mini-css-extract-plugin or extract-css-chunks-webpack-plugin to extract the compiled CSS into a separate file that you can load normally in your <head>
  • Hiding the body, using display:none, until styles are ready. The loaded styles can contain the style rules that will make the content visible

It turns out that our very own @tanhauhau wrote one of the top search results for css code splitting that explains how mini-css-extract-plugin works. @tanhauhau a couple questions for you since you seem to have some experience with how webpack does things:

  • Does simply having mini-css-extract-plugin insert the link get rid of the FOUC?
  • How do you handle SSR?

@tanhauhau
Copy link
Member

mini-css-extract-plugin extracts the css into separate chunks, similar to the js chunk. and whenever js chunk is imported, it will ensure the css chunk gets loaded to the browser first.

conceptually when you have this in the source code:

import('./Foo.svelte').then(Foo => {
  //
});

it will turn into

Promise.all([
   import('./Foo.js'),
   loadCss('./Foo.css')
]).then(([Foo]) => {
   //
});

function loadCss(css) {
   return new Promise((resolve, reject) => {
       const link = document.createElement('link');
       link.href = css;
       link.onload = resolve;
       link.onerror = reject;
       document.head.appendChild(link);
   });
}

For a client-side SPA, the HtmlWebpackPlugin will inject the entry css into head and entry js into end of the body.

<html>
    <head>
         <link rel="stylesheet" href="entry.css" />
    <head>
     <body>
          <script src="entry.js"></script>
     </body>
</html>

because the css is blocking, you kinda guaranteed that the style is there when you evaluating the entry.js, thus no FOUC.

now things get tricky with server rendering,

  • you need the styles to be send down together with the rendered html -> this will ensure your first content is styled
  • you want to avoid double downloading of the css, don't download again of what is already available
  1. sending down the styles together with rendered html.
    if you put a <link href="xxx.css" rel="stylesheet" /> then you will see a FOUC when fetching the styles.

so you have to inline the styles instead.

<style data-href="xxx.css">
   //...
</style>

this way, the styles are available and evaluated before parsing the html body.

but how do you know what are the css files needed for the page, that will be identified based on the routes / js chunks used to render the content in server.

  1. avoid double downloading of the css.
    you need to which css files is already available. we can use style[data-href] to figure that out.

min-css-extract-plugin actually does that. see https://github.com/webpack-contrib/mini-css-extract-plugin#using-preloaded-or-inlined-css.

before appending the <link href="xxx.css" rel="stylesheet" /> to the html, it will first check is there style[data-href="xxx.css"] available on the dom. if yes, then it will skip adding the <link> tag.

@benmccann
Copy link
Member Author

Would static imports get put into promises or converted to dynamic imports so that they can be run in parallel with loadCss?

E.g. today in hn.svelte.dev the Rollup outputs __sapper__/build/client/[p].8fa69d4d.js which starts with:

import{S as t,i as s,s as r,c as a,a as e,m as n,t as p,b as o,d as c}from"./client.0422cf2a.js";import"./api.5c9dd840.js";import"./index.37804c28.js";import{H as i}from"./Home.26cab2fc.js";

@tanhauhau
Copy link
Member

so we are sending import into the browser?
well, import statements are fetch in parallel, but they are evaluated in sequence right?

so you can have

import "./api.5c9dd840.css";
import "./index.37804c28.css";
import "./Home.26cab2fc.css";

import{S as t,i as s,s as r,c as a,a as e,m as n,t as p,b as o,d as c}from"./client.0422cf2a.js";
import"./api.5c9dd840.js";
import"./index.37804c28.js";
import{H as i}from"./Home.26cab2fc.js";

@Conduitry
Copy link
Member

I haven't thought through all the implications of this, but: If this made it so that no styles at all were loaded if the user had javascript disabled or if it crashed, this change would be pretty likely to be unacceptable. Extracting to real CSS files isn't just to keep the JS files down in size, it's to make sure the site degrades somewhat gracefully without javascript. Would that be possible with this?

@benmccann
Copy link
Member Author

@tanhauhau I'm wondering how it gets transformed by webpack. I think there's not really such a thing as import of a css file in the browser, but you can write it and the bundler will convert it to something like loadCss. You shared an example of what that looks like for dynamic imports and I was wondering what the transformed code looks like for static imports as well

@Conduitry good thing to check on. Yes, it would degrade gracefully. During SSR it renders a style tag into the page, so it would still work without JS on the client

@tanhauhau
Copy link
Member

ok i think im confused for a bit. so is this going to be the initial entry file, or a lazy chunk?

in webpack, things are quite different because there's a module loading mechanism in webpack.

it will ensure dependencies are loaded before executing it.

so if __sapper__/build/client/[p].8fa69d4d.js is a lazy chunk,

we can have a corresponding .css file

// filename: __sapper__/build/client/[p].8fa69d4d.css.js
export default function() {
   return Promise.all([
       loadCss("./api.5c9dd840.css"),
       loadCss("./index.37804c28.css"),
       loadCss("./Home.26cab2fc.css"),
   ]);
}

thus

Promise.all([
   import('./[p].8fa69d4d.js'),
   import('./[p].8fa69d4d.css.js').then(mod => mod.default()),
]).then(([Foo]) => {
   //
});

if it is the entry chunk, then, the css should be already been inlined in the html from ssr.

@benmccann
Copy link
Member Author

Not necessarily an entry chunk, but not dynamically imported either. Most imports in Sapper with Rollup are ES6 static imports.

I just realized this question might not make sense in the Webpack context because it looks like Webpack doesn't have the ability to output ES6 like Rollup does. I'm having a pretty hard time figuring out how to chunk and load CSS in Rollup in a way that's analogous to Webpack as a result, and so perhaps that shouldn't be the goal. I'll have to sleep on it. I think I know enough about how Webpack works now though to better understand some ways this might be approached. Interested if folks have any ideas on what the optimal way to do this in Rollup would be. Thanks again for all the help!

@benmccann
Copy link
Member Author

The Sapper CSS Rollup plugin was updated to implement the functionality suggested in the RFC and I've updated the RFC accordingly to document the design motivation for posterity. The functionality still lives within Sapper and we could probably still benefit from moving it out in order to close out this effort

@benmccann
Copy link
Member Author

Closing this since we've moved on from Sapper to SvelteKit

@benmccann benmccann closed this Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants