nystudio107 / rollup-plugin-critical

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

Critical + Penthouse passdown HTML issue #8

Closed davidwebca closed 1 year ago

davidwebca commented 1 year ago

Describe the bug

I was trying to generate critical CSS and I was wondering why only some of my CSS variables were output and no selector at all. The issue was that critical loads the HTML incorrectly through vinyl by getting the HTML document and passing it down as a string to Penthouse. With a server app or SSR, it wouldn't be an issue, but with an SPA with Vue, this seems to prevent Penthouse to execute the scripts and no vu component gets rendered or picked up.

I have tested to make sure that Penthouse does generate the correct HTML and loads the right page and it seems to be okay. By flagging all the debugs inside rollup-plugin-critical, critical and penthouse, I've managed to see that the HTML gets fetched by critical first and passed down instead of letting penthouse load it. 🤔 I wonder if there's a reason for that.

To reproduce

Steps to reproduce the behaviour:

  1. Use the recommended settings from rollup-plugin-critical
  2. Generate a Vue app with vite
  3. Critical CSS only contains general html, body etc. styles and CSS variables

Expected behaviour

Critical CSS should be generated according to the actual HTML generated in the page.

Versions

davidwebca commented 1 year ago

I ended up whipping something fast together to go straight to penthouse.

const penthouseCriticalCss = () => {
    return {
        name: "criticalPenthouse",
        enforce: "post",
        apply: "build",
        async generateBundle(opts, bundle) {
            const criticalPages = [
                {
                    url: "http://mywebsite.test",
                    template: "home/Home",
                },
            ];

            const cssAssets = Object.keys(bundle).filter(
                (i) => bundle[i].type == "asset" && bundle[i].fileName.endsWith(".css")
            );
            if (cssAssets.length <= 0) {
                return;
            }

            let allCssString = "";
            for (let i = 0, il = cssAssets.length; i < il; ++i) {
                let cssFile = cssAssets[i];
                allCssString += ";" + bundle[cssFile].source;
            }

            for (let i = 0, il = criticalPages.length; i < il; ++i) {
                let criticalPage = criticalPages[i];

                if (allCssString) {
                    penthouse({
                        width: 1920,
                        height: 1080,
                        url: criticalPage.url,
                        cssString: allCssString,
                        blockJSRequests: false,
                        renderWaitTime: 8000,
                    }).then((criticalCssString) => {
                        outputFile(`../web/dist/criticalcss/${criticalPage.template}_critical.min.css`, criticalCssString);
                    });
                }
            }
        }
    }
}

export default defineConfig(({ command }) => ({
    cssCodeSplit: false,
    plugins: [penthouseCriticalCss()]
}))
khalwat commented 1 year ago

Do you think this issue makes sense to be posted on the critical repo?

rollup-plugin-critical just calls down through to critical to do its thing. I think the issue lies there, not in this Rollup plugin?

davidwebca commented 1 year ago

Honestly, you're right. I was seeing double while researching this issue because of the entanglement between those four dependencies (this plugin -> critical -> penthosue -> puppeteer). I'll check that out tomorrow morning. Still wanted to share my solution in case someone else stumbles onto this andif this plugin wanted explore the idea of bypassing critical altogether.

khalwat commented 1 year ago

Yeah I was thinking about that, but I'm unclear what the implications would be of bypassing the critical layer it's using now.

khalwat commented 1 year ago

Also... question... if it's an SPA, is critical CSS actually useful? Won't the page block on rendering the Vue components anyway?

davidwebca commented 1 year ago

Navigation, heros and headers - everything above the fold - are rendered on the server with inline critical CSS for a faster initial page load and everything lower is loaded through Vue. Vue is leveraged after that to allow page transitions (spa "feel" smh) and it removes what was server loaded above the fold for the following navigations.

davidwebca commented 1 year ago

Precision: There are elements that are added with JS asynchronously before vue is mounted that are fixed at the top of the page and need to have their CSS applied fast too.