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

💡 RFC: a way to inline css #918

Closed
3 tasks
kosich opened this issue Jul 28, 2021 · 12 comments
Closed
3 tasks

💡 RFC: a way to inline css #918

kosich opened this issue Jul 28, 2021 · 12 comments

Comments

@kosich
Copy link

kosich commented Jul 28, 2021

Background & Motivation

I'd like to be able to inline styles into generated .htmls

Currently my astro project (v0.17.3) generates a single shared css file. This file is required to render current page and my website network waterfall chart looks like this:

[index.html] -------------[#########]
[shared.css]                         ----[#########]
                                                        | Render

Loading styles via <link rel="stylesheet" … /> delays page render by around a hundred ms (obviously it depends on many details, but it is delayed). This inlining will likely support the "optimised by default" idea (well, with some threshold used).

In reality .html + .css size would be around 10kb, which seem to be ok for an average user to download on navigation. (That's a shared css, per-html individual css might be even less. Also, it's a tailwind styles — a custom css might be smaller)

Proposed Solution

Possible solutions

A config option to inline css in relevant html-s (maybe, with some threshold limitations)

Alternatives considered

Risks, downsides, and/or tradeoffs

The downside is that pretty much the same stylesheet would be injected in each, say, blogpost, and therefore downloaded by the client.

Open Questions

Detailed Design


Thanks

Help make it happen!

  • I am willing to submit a PR to implement this change.
  • I am willing to submit a PR to implement this change, but would need some guidance.
  • I am not willing to submit a PR to implement this change.
@eyelidlessness
Copy link
Contributor

I’m usually the odd one out who prefers external .css for the caching benefits (I’d rather block 100ms once and optimize for the next page load than bloat every page), and I’m surprised Astro doesn’t inline just based on that seeming to be the majority preference.

But even so, I definitely use inlined CSS for critical styles, especially for cajoling browsers into aggressively loading fonts when various meta tags just don’t cut it.

I think there’s a good opportunity to handle this in-module without requiring a separate config, on a per-<style> basis, eg with a type="text/css+critical" attribute (just an example but sticks to existing HTML and HTTP/MIME conventions). That said I don’t know how big an ask that is as I’m unfamiliar with that aspect of Astro’s build.

@jasikpark
Copy link
Contributor

https://github.com/GoogleChromeLabs/critters could be cool to figure out how to integrate xD

@radiosilence
Copy link

I wonder if tailwind could be JIT'd for each page in this instance?

@kosich
Copy link
Author

kosich commented Jul 29, 2021

I worked around it by using netlify's plugin netlify-plugin-inline-critical-css. Though it turned out that that plugin simply wraps another cool package, called critical — it seem to be able to take in a .html and output a modified inlined version of it.

So I a bit rewrote that netlify plugin source code, and it worked.

Here's a little instruction to add a css-inlining post-build step:

  1. Create a cssinline.js file in the project root:
let glob = require('glob');
let critical = require('critical');

// dimensions to test critical css on
let dimensions = [
  { width: 414, height: 896 },
  { width: 1920, height: 1080 }
]

// relative build output folder
let base = 'dist';

glob('/**/*.html', { root: base }, (err, files) => {
  if (err) throw err;

  files.forEach(filePath => {

    critical.generate({
      base,
      // Overwrite files by passing the same path for `src` and `target`.
      src: filePath,
      target: filePath,
      inline: true,
      minify: true,
      extract: false,
      dimensions,
      // Force critical to run penthouse only on a single page at a time to
      // avoid timeout issues.
      concurrency: 1,
    });

  });

});
  1. Install new dev dependencies:
npm i -D glob critical
  1. Run this file w/ node after the build:
node ./cssinline.js

^ you might want to add this to the npm build script: build: astro build && node ./cssinline.js

NOTE: it wont clean up that shared css, it will only inject the critical css and unblock rendering from that shared css.
NOTE 2: I haven't heavily tested this, it seem to work fine with a single shared css, built w/ tailwind


This is just a quick-fix, it doesn't fully solve the issue. A proper solution, imho, should incorporate @eyelidlessness , @jasikpark , and @radiosilence points.

@FredKSchott
Copy link
Member

FredKSchott commented Aug 3, 2021

RFC Call: Design Brainstorming

The Component Decides

  • cons: does the component really know / have the proper context?
  • lots of concern on this solution
<style critical>
  ...
</style>
<style>
  ...
</style>

The Consumer Decides

  • cons: do we want another directive for this?
  • is it better to automate via config rules?
<MyComponent style:critical />

The Automated Approach

  • Critters, etc.

Overall

  • Interesting approach, interest in adding something like this
  • open questions around how this would be added, need more info to decide
  • A proof of concept of the automated approach would go a long way to showing how easy/hard it would be!
  • can be prioritized as we get closer to v1.0, since it is mainly an optimization and doesn't impact core Astro behavior/syntax

@aresn
Copy link
Contributor

aresn commented Aug 4, 2021

We have created our own optimization script that takes care of some of the optimization we needed in order to get 100 for web vitals. This could be a snowpack plugin or part of Astro build process.

Below is a simplified version of the script we use in our deployment pipeline.

  1. Inline all external styles. (GoogleChromeLabs/critters inlines all external stylesheets and not just critical styles)
  2. Remove external styles (Critters doesn't do that because of Single Page Applications, we can because we are not SPA)
  3. Minify inlined CSS/JS. This takes care of manually added scripts (Analytics, etc.) also removes whitespace from Astro scripts.
  4. Further css optimization by "clean-css". -> minifyCSS: {level: 2}

With the above optimization, our average css coverage went from 40% to 95% and we improved our web vitals by an average of 7 points across our staging site.

const Critters = require('critters');
const path = require('path');
const fs = require('fs');
const fse = require('fs-extra');
const { walk } = require('@root/walk');
const { minify } = require('html-minifier-terser');

function removeExternalStyles(html) {
  return html.replace(/<link rel="stylesheet" href=".*\.css"[^>]+>/g, '');
}

const optimize = async function () {
  // inline @fonts in style
  const c = new Critters({
    inlineFonts: true,
    path: './dist',
    publicPath: '/',
    preload: 'body',
  });

  await fse.remove('./optimized');
  await walk('./dist', async (err, pathname, dirent) => {
    if (err) {
      throw err;
    }
    if (dirent.isDirectory() && dirent.name.startsWith('.')) {
      return false;
    }
    if (!dirent.name.match(/.*\.html$/)) {
      if (dirent.name.indexOf('.') !== -1) {
      //copy assets to optimized folder
        await fse.copy(pathname, pathname.replace('dist', 'optimized'));
      }
      return;
    }

    const html = fs.readFileSync(pathname, 'utf-8');
    const res = await c.process(html);
    const optimizedPath = pathname.replace('dist', 'optimized');
    // remove external styles, since critters doesn't actually remove the inline styles
    const htmlNoExternalStyle = removeExternalStyles(res);
    const minifiedHtml = minify(htmlNoExternalStyle, {
      removeComments: true,
      minifyJS: true,
      minifyCSS: {
        level: 2,
      },
    });

    await fse.outputFile(optimizedPath, minifiedHtml);
    console.log('name:', dirent.name, 'in', path.dirname(pathname));
  });

};

@drwpow
Copy link
Member

drwpow commented Sep 10, 2021

This could be a snowpack plugin or part of Astro build process.

I 100% agree. I think the best solution for users and for Astro is for the production build to own this optimization, since IMO this type of optimization can only happen in a context where everything is being bundled at once (providing this via a plugin probably wouldn’t work very well).

User API
Re @FredKSchott’s “The Component Decides vs The Consumer Decides” API above, my 2¢ is that only a top-level page is going to know what needs to be “critical“ for that request. It’s probably very likely that you want to critically inline a component’s styles if they appeared high up on one page, but not for the page they’re buried on (or a page several clicks in after a user has already cached all the styles). So I think a universal <style critical> is not nearly as precise as you’d want in all cases (and the entire goal of an optimization like this is surgical precision).

So maybe that does look like a <MyComponent styles:critical> only accessible via the top-level page (and maybe that cascades)? But also, maybe a page wants to declare its own <style critical> styles as well, provided that, too only works at the top-level.

I think a big question is can styles:critical be context-dependent? Is it clear that something only works within a page context, and not another? Or should we go for more of an approach like export async getCriticalCSS() (an exported function that only works for top-level pages, and will let you extract styles from Astro components as well as add arbitrary CSS as well)?

@aresn
Copy link
Contributor

aresn commented Sep 10, 2021

Drew, I just wanted to share our own experience here. We tested two different approaches. One of them out performed the other, in every way.

  1. Inline just the critical css (above the fold), then async load the rest of external css
  2. Inline/embed enough css to render just one page! remove everything else, including external css, etc.

The second approach means that every single HTML we generate has just the required css and nothing more. We do not have to worry about unused css, or the "Cumulative Layout Shift" that results from approach 1!

On average the embed css added around 2kb (gzip) to the initial HTML doc, but in returned simplified our entire process (did not have to worry what is a critical component and what is not) and increased our web vital scores by an average of 7 points. [I also like to add that we later introduced more optimizations to our build process (minimize css variable names.. etc.) ]

Components can be used above the fold, below the fold and everywhere in-between. Deciding what components are going to be above the fold and hence has critical css is just another thing the dev team has to keep track. This might introduces extra complexity to design and development.

@drwpow
Copy link
Member

drwpow commented Sep 10, 2021

We do not have to worry about unused css, or the "Cumulative Layout Shift" that results from approach 1!

Deciding what components are going to be above the fold and hence has critical css is just another thing the dev team has to keep track. This might introduces extra complexity to design and development.

That‘s extremely interesting! I’m generally in favor of any approach that reduces dev complexity. But reducing dev complexity and performance! What’s there not to like? 🙂

@jasikpark
Copy link
Contributor

jasikpark commented Sep 11, 2021

This sounds somewhere in a similar area to https://web.dev/granular-chunking-nextjs/! (found it in https://web.dev/introducing-aurora/)

@FredKSchott
Copy link
Member

Hey everyone! Our current RFC process is beginning to break down at this size, with over 50 open RFCs currently in the "discussing" stage. A growing community is a great problem to have, but our ability to give you RFC feedback has suffered as a result. In an effort to improve our RFC process, we are making some changes to better organize things.

From now on, all RFCs will live in a standalone repo: https://github.com/withastro/rfcs

This allows us to do three things: 1) Use threaded discussions for high-level ideas and improvements, without necessarily requiring an implementation for every idea. 2) Improve the quality of our RFC template and the speed/quality of all feedback. 3) Support inline comments and explicit approvals on RFCs, via a new Pull Request review process.

We hope that this new process leads to better RFC weekly calls and faster feedback on your RFCs from maintainers. More detail can be found in the new RFC repo README.


We can't automatically convert this issue to an RFC in the new repo because new RFC template is more detailed that this one. But, you can still continue this discussion in the new repo by creating a new Discussion in the RFC repo and copy-and-pasting this post (and any relevant follow-up comments) into it. Discussions are available for high-level ideas and suggestions without the requirement of a full implementation proposal.

Then, when you are ready to propose (or re-propose) an implementation for feedback and approval, you can create a new RFC using the new RFC template. More detail about how to do this can be found in the new RFC repo README.

Thanks for your patience as we attempt to improve things for both authors and reviewers. If you have any questions, don't hesitate to reach out on Discord. https://astro.build/chat

@pierredewilde
Copy link

Follow up withastro/roadmap#343

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

No branches or pull requests

8 participants