Legacy plugin inlines CSS

lukeed commented 3 years ago

⚠️ IMPORTANT ⚠️ Please check the following list before proceeding. If you ignore this issue template, your issue will be directly closed.

[x] Read the docs.
[x] Use Vite >=2.0. (1.x is no longer supported)
[x] If the issue is related to 1.x -> 2.0 upgrade, read the Migration Guide first.

Describe the bug

When attaching @vitejs/plugin-legacy, the index.css content is inlined into the index-legacy.[hash].js output file. This is reproducible with the base Preact template (and presumably others).

// index-legacy.[hash].js
var __vite_style__=document.createElement("style");__vite_style__.innerHTML="body,html{height:100%;width:100%;padding:0;margin:0;background:#fafafa;...",document.head.appendChild(__vite_style__) // ...`

truncated CSS for brevity

No changes/issues with the non-legacy output.

Reproduction

https://github.com/lukeed/bug-vite-legacy-inline-css

System Info

vite version: 2.0.1
Operating System: Mac OS
Node version: 14.15.5
Package manager (npm/yarn/pnpm) and version: pnpm 5.11.1

Logs (Optional if provided reproduction)

Don't see anything useful within --debug output. Providing summary instead:

dist/assets/index-legacy.13201260.js       4.47kb / brotli: 1.82kb
dist/assets/vendor-legacy.14fe4df4.js      8.71kb / brotli: 3.26kb
dist/assets/polyfills-legacy.1b3a07f4.js   40.77kb / brotli: 14.08kb
dist/assets/favicon.17e50649.svg   1.49kb
dist/index.html                    1.09kb
dist/assets/index.e8186b7a.css     0.35kb / brotli: 0.17kb
dist/assets/index.2844346d.js      4.51kb / brotli: 1.87kb
dist/assets/vendor.cc9d1722.js     8.64kb / brotli: 3.27kb

yyx990803 commented 3 years ago

This is somewhat intentional - right now the modern build preloads the js chunk in parallel to the css using a preload directive. In the legacy build, this will have to be done with an XHR request (which I didn't bother to implement).

I don't think this breaks anything behavior wise, but it surely can be improved.

lukeed commented 3 years ago

Yeah, it doesn't break anything, but it duplicates all of the CSS that's already included via the <link rel=stylesheet/> in the markup. Granted, this doesn't apply beyond the initial payload... but maybe deduping the CSS from initial/main entrypoint is all that needs to be done?

Bocom commented 3 years ago

Would love an option to not have the CSS inlined in the legacy build since I'm integrating it in a backend-based project where I don't have an index.html entrypoint to transform and thus enqueue the JS and CSS manually.

Connum commented 2 years ago

I would be willing to look into this and make a PR adding that option. But so far, I even fail to see where this inlining is happening - it doesn't seem to come from vite or vite-plugin-legacy (unless I missed it). So it seems so happen in one of the dependencies? Any hint would be welcome!

Update: Ok, just a few seconds after posting this, I stumbled upon #4448 - this seems related and I know where to look further now. However, I think a fix should be found that covers both the issues, using the legacy plugin or not.

andronocean commented 2 years ago

Same problem here. Backend-based PHP project, enqueuing asset links manually, the inline styles only appear in the index-legacy.abc12345.js file.

I was however able to track down where this is coming from. It's in Vite's core code directly, not the legacy plugin, and is triggered when CSS code-splitting is enabled. The CSS chunk gets produced by these lines:

https://github.com/vitejs/vite/blob/88ded7f16382d83603511de043785e01ee1e4a3a/packages/vite/src/node/plugins/css.ts#L434-L454

If you set config.build.cssCodeSplit = false, the inlining no longer occurs. It's an unfortunate tradeoff, but until a more direct control is available this is your best workaround.

I'm wondering, though... is it actually correct behavior for the whole stylesheet to get inlined here? Shouldn't it only be the aggregated CSS chunks imported in async modules?

andronocean commented 2 years ago

Update: turns out there's a bug that causes CSS files to be omitted from the manifest if build.cssCodeSplit is set to false... which could make this a real pain for backend projects. See #3629 and #6477 .

Given that, I can't see a clean workaround. We need an option to turn off the CSS inlining entirely, or only include the async chunks instead of the whole stylesheet.

k1sul1 commented 2 years ago

It'd be really nice to have the option to remove CSS from the legacy file altogether.

I also have a PHP application, WordPress to be spesific.

add_action('wp_enqueue_scripts', function() use ($app, $localizeData) {
  $build = $app->manifests['vite'];

  // Legacy bundle for browsers that don't support modules.
  // Only gets downloaded by older browsers
  $legacyPolyfill = $build->enqueue('vite/legacy-polyfills')[0];
  $handles = $build->enqueue('src/js/app-legacy.js', ['react', 'react-dom', $legacyPolyfill]);

  // This is the modern bundle. This one also enqueues CSS.
  $handles = $build->enqueue('src/js/app.js', $handles[0]);

  // localize_script(...
}

This inlining results in my legacy bundle being much larger than it otherwise would be. The nomodule attribute ensures that modern browsers won't waste their time so it doesn't even matter, but chasing perfection is really rough with an issue like this.

ermolaev commented 1 year ago

Hello @sapphi-red, recently you fix: allow tree-shake glob eager css in js Looks like it's a similar issue in legacy plugin maybe you will have time to fix it

Thanks

zhump commented 1 year ago

The css-js file compiled by non-modern browsers will write the styles twice, and the volume will increase sharply, which should be a problem.

example： dist/assets/style-legacy.e8e8324b.js

System.register([], function (e, t) {
  "use strict";
  var n = document.createElement("style");
  return (
    (n.innerHTML = "table{color:red},xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n"),
    document.head.appendChild(n),
    {
      execute: function () {
        e("default", "table{color:red}xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n");
      },
    }
  );
});

lenovouser commented 1 year ago

I would also like to see this fixed in a way where I can disable legacy output for CSS, since I already use autoprefixer which is enough legacy compatibility for me.

I have written a small plugin that seems to do the trick for me right now, but please be cautious with using it since I do not understand the inner workings of vite or rollup very well, so it might cause other bugs. It does remove the legacy CSS from the manifest.json and the build output, which is what I wanted, but I do not know if it potentially breaks other things, just to state it again.

import type { Plugin } from 'vite';

export const removeLegacyCss = (): Plugin => {
    return {
        name: 'remove-legacy-css',
        enforce: 'post',
        generateBundle: (_, bundle) => {
            const remove = Object.entries(bundle)
                .filter(([key, value]) => {
                    if ('facadeModuleId' in value) {
                        return key.includes('legacy') && value.facadeModuleId?.endsWith('css');
                    }

                    return false;
                })
                .map(([id]) => {
                    return id;
                });

            for (const key of remove) {
                delete bundle[key];
            }
        },
    };
};

It is then usable like any normal imported plugin, by adding it to the plugins array: plugins: [ removeLegacyCss() ]

lenovouser commented 1 year ago

@bocom - Would love an option to not have the CSS inlined in the legacy build since I'm integrating it in a backend-based project where I don't have an index.html entrypoint to transform and thus enqueue the JS and CSS manually.

@k1sul1 - It'd be really nice to have the option to remove CSS from the legacy file altogether.

@andronocean - Given that, I can't see a clean workaround. We need an option to turn off the CSS inlining entirely, or only include the async chunks instead of the whole stylesheet.

I also think adding an option to

a) Disable the legacy plugin for SCSS/CSS etc.; or
b) Disable the CSS inlining (not sure if these two points are really that different)

would make a lot of sense for cases like these? For backend integration it breaks stuff for me currently. I already run autoprefixer through postcss over my CSS and then include the generated files directly via <link rel="stylesheet" /> tags. For me that is fine enough.

Is one of the options acceptable to the Vite team? I think one of us would surely create a PR if there is confirmation that we are good to go with that.

reynotekoppele commented 1 year ago

I've fixed this by creating a manual chunk for each CSS file I have. This allows me the have more than one stylesheet while also omitting all the CSS in my legacy chunks. It will however generate an extra JS file with just the CSS. But those can just be ignored.

manualChunks: ( id ) => {
  if ( id.endsWith( 'scss' ) ) {
    return path.parse( id ).name;
  }
},

ext commented 1 year ago

Being hit by this issue as well. In general I'm not very fond of the approach used by this plugin as essentially having two builds takes twice the time, twice the space and it creates hard-to-track-down bugs "works on my machine" where a user and developer might run different variants of the build.

Anyway, this issue feels like a showstopper. If anyone can give some overall pointers I would gladly create a PR to fix the issue by just excluding the CSS from the legacy bundle (as it is still loaded normally using <link rel="stylesheet" src="./assets/.."> unrelated to module/legacy).

I've fixed this by creating a manual chunk for each CSS file I have. This allows me the have more than one stylesheet while also omitting all the CSS in my legacy chunks. It will however generate an extra JS file with just the CSS. But those can just be ignored.

How did you "ignore" the files? They are still referenced by System.import(..) in the legacy build, did you find a way to remove it from there?

niksy commented 1 year ago

This is how we managed to remove unecessary inlined styles when Vite emited those same styles as separate file.

Following plugin call will process only chunks inside entry-points directory ending with .scss:

function removeLegacyStyleInject({ include }) {
  let config;
  return {
    configResolved(_config) {
      config = _config;
    },
    async renderChunk(code, chunk) {
      if (
        code.includes("__vite_style__") &&
        include(chunk)
      ) {
        const t = babel.types;
        const result = await babel.transformAsync(code, {
          sourceMaps: config.build.sourcemap,
          plugins: [
            {
              visitor: {
                Identifier(path) {
                  if (path.node.name.includes("__vite_style__")) {
                    const found = path.findParent((path) => {
                      return (
                        path.isVariableDeclaration() ||
                        path.isExpressionStatement()
                      );
                    });
                    found?.remove();
                  }
                  if (path.node.name.includes("exports")) {
                    const found = path.findParent((path) => {
                      const args = path.get("arguments");
                      return (
                        path.isCallExpression() &&
                        path.get("callee").node?.name === "exports" &&
                        args.length === 2 &&
                        args[1].isStringLiteral()
                      );
                    });
                    found?.get("arguments")[1].replaceWith(t.stringLiteral(""));
                  }
                },
              },
            },
          ],
        });
        return { code: result.code, map: result.map };
      }
      return null;
    },
    enforce: "post",
    apply: "build",
  };
}

// Add this as Vite plugin
removeLegacyStyleInject({
  include: (chunk) => {
    return (
      chunk.facadeModuleId?.includes("/entry-points/") &&
      chunk.facadeModuleId?.includes(".scss")
    )
  }
})

adbenitez commented 10 months ago

I don't think this breaks anything behavior wise, but it surely can be improved.

it does! I am using the legacy plugin with renderModernChunks: false and it is noticeable when user open my app that first a white screen appears quickly for some seconds then the css is loaded by javascript, I am using vite to create and package webxdc.org apps and size is constrained so I can't afford the duplicate data

roshea1-chwy commented 8 months ago

Also getting bit by this. We're trying to use the plugin in an application where a traditional (Java) backend renders the HTML and generates script/link tags manually by reading the manifest to know which static CSS/JS imports are used by a given entrypoint (we don't require modern browser feature support and don't ship ES Modules in production). Due to this behavior, I have to choose between shipping JS that's compatible with our supported browser list or splitting CSS into its own set of chunks. We want to do both, which is possible in Webpack via separate loader chains for scripts/styles, but not via this plugin currently.

limuyaobj commented 5 months ago

Has this issue been forgotten? It seems like it hasn't been optimized for a long time.

front-refined commented 1 month ago

any update?

vitejs / vite