Skip to content
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

Legacy plugin inlines CSS #2062

Open
3 tasks done
lukeed opened this issue Feb 17, 2021 · 17 comments · May be fixed by #9920
Open
3 tasks done

Legacy plugin inlines CSS #2062

lukeed opened this issue Feb 17, 2021 · 17 comments · May be fixed by #9920
Labels

Comments

@lukeed
Copy link

lukeed commented Feb 17, 2021

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

  • Read the docs.
  • Use Vite >=2.0. (1.x is no longer supported)
  • 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
Copy link
Member

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.

@yyx990803 yyx990803 added enhancement New feature or request and removed pending triage labels Feb 19, 2021
@lukeed
Copy link
Author

lukeed commented Feb 19, 2021

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
Copy link

Bocom commented Mar 28, 2021

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
Copy link

Connum commented Aug 10, 2021

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
Copy link

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:

} else if (!config.build.ssr) {
// legacy build, inline css
chunkCSS = await processChunkCSS(chunkCSS, {
inlined: true,
minify: true
})
const style = `__vite_style__`
const injectCode =
`var ${style} = document.createElement('style');` +
`${style}.innerHTML = ${JSON.stringify(chunkCSS)};` +
`document.head.appendChild(${style});`
if (config.build.sourcemap) {
const s = new MagicString(code)
s.prepend(injectCode)
return {
code: s.toString(),
map: s.generateMap({ hires: true })
}
} else {
return { code: injectCode + code }
}

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
Copy link

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
Copy link

k1sul1 commented Apr 26, 2022

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
Copy link

ermolaev commented Aug 9, 2022

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
Copy link

zhump commented Oct 13, 2022

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
Copy link

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
Copy link

@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
Copy link

reynotekoppele commented Apr 18, 2023

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
Copy link

ext commented May 3, 2023

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
Copy link
Contributor

niksy commented May 3, 2023

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
Copy link

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
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.