gatsbyjs / gatsby

The best React-based framework with performance, scalability and security built in.
https://www.gatsbyjs.com
MIT License
55.27k stars 10.31k forks source link

Why gatsby-plugin-sass adding the stylesheet inline ? #1526

Closed creotip closed 6 years ago

creotip commented 7 years ago

the style is added like this in production also:

<style id="gatsby-inlined-css">
body { color: red; }
</style>

what is the purpose for this ? lets say i want to put my compiled css into cdn... i can't if it's inline

cr101 commented 7 years ago

Perhaps the reason is that minimizing the number of HTTP requests you pages load faster

jbolda commented 7 years ago

That is correct to my knowledge. Small amounts of css are solid this way.

cr101 commented 7 years ago

The only downside is that it gets a bit ugly under the hood when importing a CSS framework like Bootstrap. Also, it shouldn't slow the google bot crawling your site since the gbot will easily recognize the style tag and skip it.

jbolda commented 7 years ago

With the advent of http2 push, it may make sense to include those as separate files that they get a cache separate from your homepage which may change more often. In that case, you can use https://www.gatsbyjs.org/docs/adding-images-fonts-files/#using-the-static-folder . You may take advantage of all of these techniques to get the best balance of load times and browser paint. (Inline css means the browser won't need to repaint as opposed to loading css seperately which may.)

cr101 commented 7 years ago

@jbolda That mechanism would be perfect if you are including the entire Bootstrap CSS but it wouldn't work if you need to customize the Bootstrap SASS which most developers using Bootstrap do.

jbolda commented 7 years ago

If that is the case, you can use the onPostBuild api to copy the modified css into the public folder and just link to it seperately.

jbolda commented 7 years ago

And at the point, the likelihood of things changing is greater and you should be stripping it down to only what you need to use. So it might be small enough and change often enough to just inline it.

benjaminhoffman commented 6 years ago

@creotip closing this issue for now as it appears your question got answered. If I'm mistaken or you have a follow-up, feel free to re-open it. Thanks!

l0co commented 6 years ago

This is answered about "what's the reason", but I'd extend the question: how to disable this feature?

It seems to be not reasonable. For example if I have a site with 20 subpages, and the whole bootstrap/theme css is of 350KB size, if it was loaded from css file using <link src='...'> it would only be loaded once and then get from the cache. Instead, it is forcefully inlined into each page html file and these 350KB are loaded separately for each page.

l0co commented 6 years ago

I've spent almost 4 hours to find the resolution. It seems they even didn't provide an option to disable this, while this is clearly the feature only useful for SPA-s.

Here is the workaround in gatsby-ssr.js which replaces all (can be further customized) inline styles with <link>:

// replace inline css/scss with links
exports.onPreRenderHTML = ({ getHeadComponents, replaceHeadComponents }) => {
    if (process.env.NODE_ENV !== 'production')
        return;

    let hc = getHeadComponents();
    hc.forEach(el => {
        if (el.type === 'style') {
            el.type = 'link';
            el.props['href'] = el.props['data-href'];
            el.props['rel'] = 'stylesheet';
            el.props['type'] = 'text/css';

            delete el.props['data-href'];
            delete el.props['dangerouslySetInnerHTML'];
        }
    })
}
koga73 commented 5 years ago

This absolutely should be optional. We are required to set the "content-security-policy" header which does not allow inline-styles. Because of the inline styles generated here we would need to set the header to "style-src unsafe-inline". With many organizations that take security seriously this would not be allowed.

fullofcaffeine commented 4 years ago

+1, should be optional.

paprikman commented 4 years ago

+1, has to be optional.

nibtime commented 4 years ago

+1, this must be configurable. I had to adapt the workaround from @l0co , it was throwing the following error during production build (Gatsby 2.19.12):

WebpackError: Minified React error #137; visit https://reactjs.org/docs/error-decoder.html?invariant=137&args[]=link&args[]= 
for the full message or use the non-minified dev environment 
for full errors and additional helpful warnings.

The adapted workaround:

import { PreRenderHTMLArgs } from "gatsby"
import React from "react"

export const onPreRenderHTML = ({ getHeadComponents, replaceHeadComponents }: PreRenderHTMLArgs): any => {
  if (process.env.NODE_ENV !== "production") {
    return
  }
  const headComponents = getHeadComponents()

  // workaround to revert forcefully injected inline styles
  // described in https://github.com/gatsbyjs/gatsby/issues/1526
  // takes data-href from <style> tag with inline styles which contains URL to global css file
  // and transforms it into stylesheet link component
  const transformedHeadComponents = headComponents.map((node: any) => {
    if (node.type === "style") {
      const globalStyleHref = node.props["data-href"]
      if (globalStyleHref) {
        return <link href={globalStyleHref} rel="stylesheet" type="text/css" />
      } else {
        return node
      }
    } else {
      return node
    }
  })
  replaceHeadComponents(transformedHeadComponents)
}
abemedia commented 4 years ago

@nibtime I had the same issue but fixed it by simply checking if el.props['data-href'] exists i.e.

export const onPreRenderHTML = ({ getHeadComponents }) => {
  if (process.env.NODE_ENV !== 'production') return

  getHeadComponents().forEach(el => {
    if (el.type === 'style' && el.props['data-href']) { // <- this was the issue
      el.type = 'link'
      el.props['href'] = el.props['data-href']
      el.props['rel'] = 'stylesheet'
      el.props['type'] = 'text/css'

      delete el.props['data-href']
      delete el.props['dangerouslySetInnerHTML']
    }
  })
}
bogdanmarcu commented 4 years ago

I'm pretty new with this so maybe I'm not getting this right ... but if I'm importing style through gatsby-browser or even in the page template it places all my css inline, which increases the file-size of my index from 5kb to 250kb. And I have 1500+ individual pages, one for each product.

This is awful. And I haven't seen anything mentioning this in documentation.

abemedia commented 4 years ago

I'm pretty new with this so maybe I'm not getting this right ... but if I'm importing style through gatsby-browser or even in the page template it places all my css inline, which increases the file-size of my index from 5kb to 250kb. And I have 1500+ individual pages, one for each product.

This is awful. And I haven't seen anything mentioning this in documentation.

Just paste the code from https://github.com/gatsbyjs/gatsby/issues/1526#issuecomment-639993049 into your gatsby-ssr.js file to fix this.

untitaker commented 3 years ago

I had to add delete el.props['children'] to get the workaround of @l0co working.

FWIW adding this workaround reduced our pre-rendered HTML size (that is, sizes of just the .html files summed up) from 1.1GB to 700MB. Given how this issue was handled I suspect there may be other low-hanging fruits like this.

sonic1981 commented 3 years ago

That is correct to my knowledge. Small amounts of css are solid this way.

worth noting that this is not ideal from a security point of view. You have to add unsafe-inline to your CSP header which is not best practice

This might of been fine in 2017, but in 2021 CSS be nonced

runemadsen commented 3 years ago

We're running into problems with the Gatsby bundle size, and I just want to +1 that this should be a configurable option.