nystudio107 / rollup-plugin-critical

Vite.js & Rollup plugin for generating critical CSS
MIT License
91 stars 11 forks source link

Wrong Critical CSS generated #10

Closed martinhellwagner closed 1 year ago

martinhellwagner commented 1 year ago

Describe the bug

We have the Rollup Critical CSS plugin in our Vite project, and the CSS files get generated by the plugin as they should. However, they contain a rather weird Critical CSS, which still leads to Flashes of Unstyled Content. For example, a run on https://www.karriere.at/lp/faqs leads to the following Critical CSS (all of our URLs generate this Critical CSS code):

:root{--swiper-theme-color:#007aff}:root{--swiper-navigation-size:44px}*,:after,:before{-webkit-box-sizing:border-box;box-sizing:border-box;border-width:0;border-style:solid;border-color:rgb(var(--c-gray-200))}:after,:before{--tw-content:""}html{line-height:1.5;-webkit-text-size-adjust:100%;-moz-tab-size:4;-o-tab-size:4;tab-size:4;font-family:OpenSans,Helvetica,Arial,sans-serif}body{margin:0;line-height:inherit}:-moz-focusring{outline:auto}:-moz-ui-invalid{box-shadow:none}::-webkit-inner-spin-button,::-webkit-outer-spin-button{height:auto}::-webkit-search-decoration{-webkit-appearance:none}::-webkit-file-upload-button{-webkit-appearance:button;font:inherit}:root{--c-black:0 0 0;--c-white:255 255 255;--c-gray-50:245 247 247;--c-gray-100:237 239 241;--c-gray-200:227 230 233;--c-gray-300:201 205 212;--c-gray-400:162 170 182;--c-gray-500:124 135 151;--c-gray-600:89 102 123;--c-gray-700:67 82 105;--c-gray-800:46 61 86;--c-gray-900:20 37 66;--c-primary-50:244 251 240;--c-primary-100:233 247 222;--c-primary-200:218 239 192;--c-primary-300:198 230 159;--c-primary-400:180 221 127;--c-primary-500:164 213 96;--c-primary-600:139 199 42;--c-primary-700:97 159 0;--c-primary-800:55 120 0;--c-primary-900:13 83 0;--c-secondary-50:230 238 250;--c-secondary-100:205 221 245;--c-secondary-200:179 205 240;--c-secondary-300:129 171 230;--c-secondary-400:104 154 225;--c-secondary-500:53 121 215;--c-secondary-600:3 87 205;--c-secondary-700:2 70 164;--c-secondary-800:2 52 123;--c-secondary-900:20 37 66;--c-tertiary-50:250 238 237;--c-tertiary-100:247 224 222;--c-tertiary-200:240 200 196;--c-tertiary-300:232 172 166;--c-tertiary-400:225 144 137;--c-tertiary-500:218 117 108;--c-tertiary-600:205 70 58;--c-tertiary-700:166 32 30;--c-tertiary-800:117 0 0;--c-tertiary-900:82 0 0}html{scroll-behavior:smooth}body{--tw-bg-opacity:1;background-color:rgb(var(--c-white) / var(--tw-bg-opacity));font-family:OpenSans,Helvetica,Arial,sans-serif;font-size:clamp(1rem,calc(1rem + 0*(100vw - 20rem)),1rem);line-height:1.5;--tw-numeric-figure:lining-nums;font-variant-numeric:var(--tw-ordinal) var(--tw-slashed-zero) var(--tw-numeric-figure) var(--tw-numeric-spacing) var(--tw-numeric-fraction);--tw-text-opacity:1;color:rgb(var(--c-gray-900) / var(--tw-text-opacity));-webkit-font-smoothing:antialiased;-moz-osx-font-smoothing:grayscale}*,:after,:before{--tw-translate-x:0;--tw-translate-y:0;--tw-rotate:0;--tw-skew-x:0;--tw-skew-y:0;--tw-scale-x:1;--tw-scale-y:1;--tw-pan-x: ;--tw-pan-y: ;--tw-pinch-zoom: ;--tw-scroll-snap-strictness:proximity;--tw-ordinal: ;--tw-slashed-zero: ;--tw-numeric-figure: ;--tw-numeric-spacing: ;--tw-numeric-fraction: ;--tw-ring-inset: ;--tw-ring-offset-width:0px;--tw-ring-offset-color:#fff;--tw-ring-color:rgb(59 130 246 / .5);--tw-ring-offset-shadow:0 0 #0000;--tw-ring-shadow:0 0 #0000;--tw-shadow:0 0 #0000;--tw-shadow-colored:0 0 #0000;--tw-blur: ;--tw-brightness: ;--tw-contrast: ;--tw-grayscale: ;--tw-hue-rotate: ;--tw-invert: ;--tw-saturate: ;--tw-sepia: ;--tw-drop-shadow: ;--tw-backdrop-blur: ;--tw-backdrop-brightness: ;--tw-backdrop-contrast: ;--tw-backdrop-grayscale: ;--tw-backdrop-hue-rotate: ;--tw-backdrop-invert: ;--tw-backdrop-opacity: ;--tw-backdrop-saturate: ;--tw-backdrop-sepia: }body{font-size:1rem;line-height:1.5;letter-spacing:-.1px;color:#142542}::-webkit-input-placeholder{color:#c9cdd4}::-moz-placeholder{color:#c9cdd4}:-ms-input-placeholder{color:#c9cdd4}*{outline-offset:.125rem}html{-webkit-box-sizing:border-box;box-sizing:border-box}*,:after,:before{-webkit-box-sizing:border-box;box-sizing:border-box}@font-face{font-weight:400;font-style:normal;font-family:Open Sans;font-display:swap;src:url(https://kcdn.at/assets/fonts/open-sans-v27-latin-ext_latin-regular.woff2) format("woff2"),url(https://kcdn.at/assets/fonts/open-sans-v27-latin-ext_latin-regular.woff) format("woff")}@font-face{font-weight:400;font-style:italic;font-family:Open Sans;font-display:swap;src:url(https://kcdn.at/assets/fonts/open-sans-v27-latin-ext_latin-italic.woff2) format("woff2"),url(https://kcdn.at/assets/fonts/open-sans-v27-latin-ext_latin-italic.woff) format("woff")}@font-face{font-weight:700;font-style:normal;font-family:Open Sans;font-display:swap;src:url(https://kcdn.at/assets/fonts/open-sans-v27-latin-ext_latin-600.woff2) format("woff2"),url(https://kcdn.at/assets/fonts/open-sans-v27-latin-ext_latin-600.woff) format("woff")}@font-face{font-weight:700;font-style:italic;font-family:Open Sans;font-display:swap;src:url(https://kcdn.at/assets/fonts/open-sans-v27-latin-ext_latin-600italic.woff2) format("woff2"),url(https://kcdn.at/assets/fonts/open-sans-v27-latin-ext_latin-600italic.woff) format("woff")}@font-face{font-family:Open Sans-fallback;size-adjust:105.44%;ascent-override:103%;src:local("Arial")}html{-webkit-text-size-adjust:100%;-moz-text-size-adjust:100%;-ms-text-size-adjust:100%;text-size-adjust:100%}body,html{margin:0;padding:0;border:0;vertical-align:baseline;font:inherit;font-size:100%}html{font-family:Open Sans,Open Sans-fallback,Arial,sans-serif;font-variant-numeric:lining-nums}body{-webkit-text-size-adjust:none}::-webkit-file-upload-button{-webkit-appearance:button;font:inherit}::-ms-clear{display:none}html{font-size:100%;-webkit-font-smoothing:antialiased;text-rendering:optimizeLegibility;scroll-behavior:smooth}body{font-size:1rem;line-height:1.5;font-family:OpenSans,Arial,sans-serif;font-weight:400;font-variant-numeric:lining-nums;letter-spacing:-.1px;color:#142542;background:#fff}

What are we doing wrong that the Critical CSS is so general (and the same for all URLs)?

To reproduce

Steps to reproduce the behaviour:

  1. Generate Critical CSS

Expected behaviour

The correct Critical CSS is produced.

Versions

martinhellwagner commented 1 year ago

@khalwat

Any idea how we could resolve this issue? 🤔

khalwat commented 1 year ago

I don't know what you're doing wrong, other than you might be passing in the wrong URLs to generate critical CSS from? Here's an example setup:

https://github.com/nystudio107/devmode/blob/develop/buildchain/vite.config.ts#L33

^ which definitely does work.

martinhellwagner commented 1 year ago

@khalwat

That's all correct, I've checked. Is it possible that the input option in rollupOptions options is incorrect? We have Javascript files as input options, which in turn link to the respective CSS files.

khalwat commented 1 year ago

You're going to have to provide more information -- your vite.config.js and also the code you're using to include the Critical CSS in your templates.

I'd suggest looking at the devmode repo above to compare & contrast.

martinhellwagner commented 1 year ago

This is our Vite Config file:

import {defineConfig} from 'vite';
import vue from '@vitejs/plugin-vue';
// import legacy from '@vitejs/plugin-legacy';
import ViteRestart from 'vite-plugin-restart';
import viteCompression from 'vite-plugin-compression';
import manifestSRI from 'vite-plugin-manifest-sri';
import {visualizer} from 'rollup-plugin-visualizer';
import eslintPlugin from 'vite-plugin-eslint';
import {nodeResolve} from '@rollup/plugin-node-resolve';
import critical from 'rollup-plugin-critical';
import {ViteFaviconsPlugin} from 'vite-plugin-favicon2';
import * as path from 'path';

// https://vitejs.dev/config/

export default defineConfig(({command, mode}) => ({
  base: command === 'serve' ? '' : (mode === 'production' ? 'https://content.karriere.at/dist/' : '/dist/'),
  build: {
    emptyOutDir: true,
    manifest: true,
    outDir: path.resolve('../web/dist'),
    rollupOptions: {
      input: {
        app: '../src-vite/js/app.ts',
        appk4: '../src-vite/js/app-k4.ts',
        appb2c: '../src-vite/js-b2c/app.js'
      },
      output: {
        sourcemap: true,
      },
    },
  },
  plugins: [
    critical({
      criticalUrl: 'https://karriere.at/',
      criticalBase: path.resolve('../src-vite'),
      criticalPages: [
        { uri: 'gehaltsstudie', template: 'templates/entry/landingpages/gehaltsstudie' },
        { uri: 'gehaltsstudie/danke', template: 'templates/entry/landingpages/gehaltsstudieDanke' },
        { uri: 'lp/events', template: 'templates/entry/landingpages/events' },
        { uri: 'lp/faqs', template: 'templates/entry/landingpages/faqs' },
        { uri: 'lp/lebenslauf', template: 'templates/entry/landingpages/lebenslauf' },
        { uri: 'lp/lebenslauf-anlegen', template: 'templates/entry/landingpages/default' },
        { uri: 'lp/lebenslauf-vorlagen', template: 'templates/entry/landingpages/vorlagen' },
        { uri: 'presse', template: 'templates/entry/pagesB2C/presseHome' },
        { uri: 'presse/downloads', template: 'templates/entry/pagesB2C/presseDownloads' },
        { uri: 'presse/medienecho', template: 'templates/entry/pagesB2C/presseMedienechoOverview' },
        { uri: 'presse/medienecho/homeoffice-auch-nach-der-krise', template: 'templates/entry/medienecho/default' },
        { uri: 'presse/pressemitteilungen', template: 'templates/entry/pagesB2C/presseOverview' },
        { uri: 'presse/pressenews-abonnieren', template: 'templates/entry/pagesB2C/presseSubscription' },
        { uri: 'presse/s/pm', template: 'templates/entry/search/resultsPresse' },
        { uri: 'presse/s/pg', template: 'templates/entry/search/resultsPresseGlobal' },
        { uri: 'presse/s/me', template: 'templates/entry/search/resultsPresseMedienecho' },
        { uri: 'presse/weiterbildung-als-weg-aus-der-krise', template: 'templates/entry/pressemitteilungen/default' }
      ],
      criticalConfig: {
        inline: false,
        width: 1680,
        height: 1050,
        penthouse: {
          timeout: 3000000,
        },
        ignore: {
          rule: [/o-footer/],
        },
        rebase: asset => (asset.url.includes('https://content.karriere.at') || asset.url.includes('https://kcdn.at')) ? `${asset.url}` : `https://content.karriere.at${asset.url}`,
      },
    }),
    nodeResolve({
      moduleDirectories: [path.resolve('./node_modules'),],
    }),
    ViteFaviconsPlugin({
      logo: './public/images/bitmap/favicon.png',
      inject: false,
      outputPath: 'favicons-default',
      favicons: {
        appName: 'karriere.at',
        lang: 'de',
        background: '#244F43',
        theme_color: '#8BC72A',
      },
    }),
    ViteRestart({
      reload: [
        '../translations/**/*.{php}',
        './templates/**/*.{twig}',
      ],
    }),
    vue(),
    viteCompression({
      filter: /\.(js|mjs|json|css|map)$/i,
    }),
    manifestSRI(),
    visualizer({
      filename: path.resolve('../web/dist/assets/stats.html'),
      template: 'treemap',
      sourcemap: true,
    }),
    eslintPlugin({
      cache: false,
    }),
  ],
  publicDir: './public',
  resolve: {
    alias: {
      '@': path.resolve(__dirname, './'),
    },
    preserveSymlinks: true,
  },
  css: {
    postcss: 'postcss.config.js',
  },
  // Use this for Laravel Valet
  server: {
    fs: {
      strict: false
    },
    host: '0.0.0.0',
    origin: 'http://localhost:3000',
    port: 3000,
    strictPort: true,
  },
}));

The newly included file (and the one whose styles don't seem to get pushed into the Critical CSS files) looks like this:

/**
 * Bundle Scripts
 */

// Polyfills
import 'babel-polyfill';
import 'svgxuse';
import objectFitImages from 'object-fit-images';

// CSS Import
import '../css-b2c/app.scss';

// Import Partials
import scripts from './partials/scripts';
import modules from './partials/modules';

const app = {
  init() {
    scripts.init();
    modules.init();
    objectFitImages();
  },
};

// Init App
app.init();
khalwat commented 1 year ago

How is this newly include file loaded?

martinhellwagner commented 1 year ago

It is loaded like this in the src-vite/js/app.ts file:

import('../js-b2c/app.js');

The src-vite/js/app.ts file is called like this in the default layout template:

{{ craft.vite.script('js/app.ts', false) }}

The CSS works on the Frontend, but I have a feeling we're doing something wrong in terms of Critical CSS?

martinhellwagner commented 1 year ago

@khalwat

Any idea how this could be resolved?

khalwat commented 1 year ago

If I had to guess, it's because you're doing a dynamic import with import(), and perhaps when Headless Chrome does the page rendering, it isn't pulling that in.

I'd suggest changing it to a regular ESM import, e.g.:

import '../js-b2c/app.js';
martinhellwagner commented 1 year ago

@khalwat

Unfortunately, that doesn't change anything. However, it doesn't even work when trying with the working Critical CSS of another project.

It is necessary to have a CSS input file specified, or is it possible that the headless Chrome browses a route on its own and just extracts the above-the-fold CSS into a Critical file?

martinhellwagner commented 1 year ago

Also, what's strange is that I'm getting the following warning, even though I'm using the exact Critical config of another project:

Warning: Missing base path. Consider 'base' option. https://goo.gl/PwvFVb

I'm not getting the warning on the other project.

martinhellwagner commented 1 year ago

@khalwat

I managed to find the cause of the error: the templatePath (which was used for the criticalPath variable) wasn't set correctly in the templates.

Thanks for your help! 🙏🏼