Skip to content

[feat] inlineCss option #2620

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

Merged
merged 31 commits into from
Jan 11, 2022
Merged

[feat] inlineCss option #2620

merged 31 commits into from
Jan 11, 2022

Conversation

untp
Copy link
Contributor

@untp untp commented Oct 17, 2021

fixes #962

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2021

🦋 Changeset detected

Latest commit: 96e824b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@untp untp changed the title feat: inline CSS [feat] inline CSS Oct 17, 2021
@benmccann benmccann changed the title [feat] inline CSS [feat] inlineCSS option Oct 25, 2021
@benmccann
Copy link
Member

Is the term for this really inlineCSS? Because if I search Google for inline CSS all the results I get look like <p style="font-size: 20px; font-weight: bold;">Some text</p>. Maybe it should be called embedCSS? Or externalizeCSS: false?

@untp
Copy link
Contributor Author

untp commented Oct 26, 2021

Is the term for this really inlineCSS? Because if I search Google for inline CSS all the results I get look like <p style="font-size: 20px; font-weight: bold;">Some text</p>. Maybe it should be called embedCSS? Or externalizeCSS: false?

Well, the main reason people want this feature is getting a low score on PageSpeed Insights/Lighthouse. Loading CSS blocks rendering and the site gets a low First Contentful Paint score. Then Lighthouse recommends:

Eliminate render-blocking resources

Resources are blocking the first paint of your page. Consider delivering critical JS/CSS inline and deferring all non-critical JS/styles. Learn more.

In web.dev's Eliminate render-blocking resources article (Learn more link):

Similar to inlining code in a <script> tag, inline critical styles required for the first paint inside a <style> block at the head of the HTML page.

They are using inline keyword. So it is better to naming it to inlineCSS.

Of course, inlining CSS can mean style attribute of an element. But docs can be written like "inline css inside a <style> block at the head of the HTML page" to clear up ambiguity.

@untp untp changed the title [feat] inlineCSS option [feat] inlineCss option Oct 26, 2021
@dummdidumm
Copy link
Member

dummdidumm commented Oct 27, 2021

Thinking out loud here: is this option granular enough? Will people always have this kind of either-or situation? Wouldn't people instead have some critical styles which should be inlined and then other styles loaded through css normally?

@dominikg
Copy link
Member

dominikg commented Oct 27, 2021

A simple boolean toggle for everything isn't a good general purpose solution for MPA. The second page re-downloads all css because it cannot be used from the cache.

So instead of inlining everything it would be better to only inline critical css to allow fast rendering above the fold without layout shift and continue to serve everything via css file - which would still result in more total bytes transferred, but not the whole css and it keeps caching viable.

there are 3rd party solutions like https://github.com/nystudio107/rollup-plugin-critical, which may be used to automate this.
It uses Addy Osmanis critical under the hood https://github.com/addyosmani/critical. He also mentioned this article about googles aurora initiative: https://web.dev/aurora-resource-inlining ( trigger warning, angular and react are mentioned ;))

Optimizing css itself is only the fist step too, adding prefetch hints for images referenced by critical css, preconnect to cdn, embedding the font (There are even tools to build a custom font only carrying the chars used on the page), ... there are so many things you can to to etch out a little more speed - only to have it ruined by a "mandatory" background video to make it "look cool" or a gazillion of tracking scripts 🙈.

In the end an optimized first visit page should carry little critical weight so that deferred bigger resources like script and style can be downloaded while the user is still busy starting the first interaction.

I wonder if instead of this boolean we could add stats/information on the sizes of entry pages, similar to bundle sizes reported by build, but at a page level, with separate thresholds and accompanied by documentation on how improve the sizes.

edit: there was a little bit of discussion about this on vite discord too https://discord.com/channels/804011606160703521/804067627852365845/868824287459549214

@untp
Copy link
Contributor Author

untp commented Oct 28, 2021

Caching is still viable, because all CSS is bundled in JS files. The first page (SSR/prerendered HTML) includes inlined CSS. When Svelte hydrates the page, it downloads JS files which include CSS styles. Then it replaces inlined CSS with the styles from the JS files. When the second page requested, Svelte downloads JS files which include CSS styles. HTML response is only used on first page. If JS files are cached, styles are cached too.

The problem with inlining only above the fold content is depending on the puppeteer package. Puppeteer can't be used on SSR, because server response time will be long, and some adapters like Cloudflare Workers can't be used. Puppeteer can be used only on build time and if it used building will be slow.

Next.js' experimental.optimizeCss option uses critters package which doesn't use a headless browser and inlines all CSS rules used by the page, rather than only those needed for above-the-fold content. This PR's option is like this, inlining all styles used by the page.

@netlify
Copy link

netlify bot commented Jan 3, 2022

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: bc79b0a

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61d57727d5f4ec00081a06a0

@Rich-Harris
Copy link
Member

Thanks — have updated this PR so that it's once again in a mergeable state.

In principle I'm in favour of this change (though I wonder if it should be a threshold rather than a toggle — if one page had a very large amount of CSS, you might want that page to use <link> rather than <style>.

Unfortunately there's one major problem. If you visit a page that has styles from components A, B and C, you'll have this...

<style>
  /* A */
  /* B */
  /* C */
</style>

...then if you navigate to a page that has components B, C and D you'll get this:

<style>
  /* A */
  /* B */
  /* C */
</style>

<link rel="stylesheet" href="/assets/B.css">
<link rel="stylesheet" href="/assets/C.css">
<link rel="stylesheet" href="/assets/D.css">

If you then navigate back to the first page, you'll load A again.

In contrast, when styles are always included as links, the first navigation will only load D, and the second navigation won't load anything at all. Aside from resulting in faster navigation and less wasted bandwidth, this also prevents a situation where the CSSOM is cluttered with duplicated selectors.

The CSS loading is done by Vite here — here's where it checks to see if styles are already in the page.

I'm not sure what the best solution to this is. Perhaps we include the links as well as the <style>, to fool Vite, but add the disabled attribute so that they remain inert?

@netlify
Copy link

netlify bot commented Jan 11, 2022

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: 96e824b

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61de03930691ed0009ac6a8e

@Rich-Harris Rich-Harris merged commit 2097f00 into sveltejs:master Jan 11, 2022
@untp untp deleted the inline-css branch January 12, 2022 07:25
@gustavopch
Copy link

gustavopch commented Jan 12, 2022

What's the unit in inlineStyleThreshold? Bytes? BTW, it may be too late, but it could have been named maxBytesToInlineStyle to make that self-evident.

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.

Can CSS be inlined in the server-rendered pages ?
6 participants