rollup-plugin-postcss icon indicating copy to clipboard operation
rollup-plugin-postcss copied to clipboard

styleInject breaks tree-shaking

Open adi518 opened this issue 5 years ago • 12 comments

This function doesn't have a /*#__PURE__*/ prefix, which breaks tree-shaking. Moreover, when I try to prepend it myself through inject option it gets stripped along with the entire string from the final bundle, which leaves you without CSS.

function inject(cssVariableName) {
  return (
    '\n' +
    `/*#__PURE__*/ (${styleInjectFork})(${cssVariableName}${
      Object.keys(styleInjectOptions).length > 0
        ? `,${JSON.stringify(styleInjectOptions, function(_key, value) {
            if (typeof value === 'function') {
              return value.toString();
            }
            return value;
          })}`
        : ''
    });`
  );
}

In my case, I forked styleInject, because it's missing some features, so I had to compose it slightly different into the string (wrap it with brackets to turn it into expression, which is then invokable).

adi518 avatar Jul 16 '20 00:07 adi518

Hi @adi518!

Sorry to hear you're running into an issue. To help us best begin debugging the underlying cause, it is incredibly helpful if you're able to create a minimal reproduction. This is a simplified example of the issue that makes it clear and obvious what the issue is and how we can begin to debug it.

If you're up for it, we'd very much appreciate if you could provide a minimal reproduction and we'll be able to take another look.

wardpeet avatar Aug 30 '20 21:08 wardpeet

Sure, I have a repro in hand. Will post it here soon. 👍

adi518 avatar Aug 31 '20 10:08 adi518

I'm guessing you're referring to tree-shaking in webpack specifically?

benmccann avatar Sep 05 '20 05:09 benmccann

Tree-shaking in every tree-shaking compatible tool (Webpack, Rollup, etc'). The issue here is that styleInject snippets (function calls) are bundled into the global scope without the pure annotation, which means they are detected as side-effects, thus breaking the shake. So, I added the annotation myself by passing a custom inject function. Alas, the entire snippet is stripped out completely, for unknown reasons. Someone who's more savvy with Rollup will probably know, hence the issue. I'll provide the repro soon.

adi518 avatar Sep 05 '20 14:09 adi518

Ah, I didn't know Rollup supported these annotations, but it looks like it does: https://rollupjs.org/guide/en/#treeshake

benmccann avatar Sep 05 '20 17:09 benmccann

I guess the problem here is that injecting CSS IS a side effect. I guess injectStyle should only be stripped out if it's injecting styles that are relevant to the JS module requiring them (like a React component). For example, a CSS Module with no :global selector can be safely marked as pure.

Is Webpack's css-loader handling this in some way? How do they do it?

dapetcu21 avatar Sep 05 '20 17:09 dapetcu21

Injecting is a side-effect yes, but we can tell the transpiler it's safe by adding the pure annotation. Babel does that automatically for React imports. Webpack doesn't add shaking measures when you build a library with it, it only knows how to read them, that's why we need Rollup. Webpack 5 might bring these features, but it's not there yet, so until then, we depend on Rollup or other ESM-aware bundlers.

adi518 avatar Sep 05 '20 20:09 adi518

Any updates?

andkh avatar Oct 28 '20 17:10 andkh

Hi, no. I was busy with other stuff. I intend to come back to this though, because I really need to solve this issue.

adi518 avatar Oct 28 '20 18:10 adi518

Just pinging this thread to see if there is any progress. This would be so great!

danlevy1 avatar Feb 26 '21 02:02 danlevy1

Currently, the output from this plugin looks like this:

var css_248z = ".___2X_Ir{background-color:#4e707a;font-size:24px;color:#fff;cursor:pointer}";
var Button_module = {"button":"___2X_Ir"};
styleInject(css_248z);

If we want the styleInject(..) function to only execute when the export is imported, we need to make the following modifications:

  1. Wrap the styleInject(..) function call in an IIFE
  2. Use the /*#__PURE__*/ annotation
  3. Instead of exporting var Button_module = {"button":"___2X_Ir"};, we need to return Button_module from the IIFE
  4. Set the result of the IIFE to a variable. We can call this variable Button_module and change the name of the style mapping variable to styleMapping.

The code would look like this:

var css_248z = ".___2X_Ir{background-color:#4e707a;font-size:24px;color:#fff;cursor:pointer}";
var styleMapping = {"button":"___2X_Ir"};
var Button_module = /*#__PURE__*/(function() {
  styleInject(css_248z);
  return styleMapping;
})();

These changes can be made inside of the src/postcss-loader.js file, and it can be put behind a flag (e.g. noSideEffects). In order to produce the IIFE code above, this is the code I used inside of src/postcss-loader.js:

if (shouldExtract) {
    output += `export default ${JSON.stringify(modulesExported[_this.id])};`;
    extracted = {
      id: _this.id,
      code: result.css,
      map: outputMap
    };
} else {
    const module = supportModules ? JSON.stringify(modulesExported[_this.id]) : cssVariableName;
        output += `var ${cssVariableName} = ${JSON.stringify(result.css)};\n` + `var styleMapping = ${module};\n` + `export var stylesheet=${JSON.stringify(result.css)};`;
}

if (!shouldExtract && shouldInject) {
    output += typeof options.inject === 'function' ? options.inject(cssVariableName, _this.id) : '\n' + `import styleInject from '${styleInjectPath}';\n` + `export default /*#__PURE__*/(function() {\n  styleInject(${cssVariableName}${Object.keys(options.inject).length > 0 ? `,${JSON.stringify(options.inject)}` : ''});\n  return styleMapping;\n})();`;
}

For others looking at this comment, I also added a feature request to a similar plugin: https://github.com/Anidetrix/rollup-plugin-styles/issues/168

danlevy1 avatar Mar 03 '21 03:03 danlevy1

Currently, the output from this plugin looks like this:

var css_248z = ".___2X_Ir{background-color:#4e707a;font-size:24px;color:#fff;cursor:pointer}";
var Button_module = {"button":"___2X_Ir"};
styleInject(css_248z);

If we want the styleInject(..) function to only execute when the export is imported, we need to make the following modifications:

  1. Wrap the styleInject(..) function call in an IIFE
  2. Use the /*#__PURE__*/ annotation
  3. Instead of exporting var Button_module = {"button":"___2X_Ir"};, we need to return Button_module from the IIFE
  4. Set the result of the IIFE to a variable. We can call this variable Button_module and change the name of the style mapping variable to styleMapping.

The code would look like this:

var css_248z = ".___2X_Ir{background-color:#4e707a;font-size:24px;color:#fff;cursor:pointer}";
var styleMapping = {"button":"___2X_Ir"};
var Button_module = /*#__PURE__*/(function() {
  styleInject(css_248z);
  return styleMapping;
})();

These changes can be made inside of the src/postcss-loader.js file, and it can be put behind a flag (e.g. noSideEffects). In order to produce the IIFE code above, this is the code I used inside of src/postcss-loader.js:

if (shouldExtract) {
    output += `export default ${JSON.stringify(modulesExported[_this.id])};`;
    extracted = {
      id: _this.id,
      code: result.css,
      map: outputMap
    };
} else {
    const module = supportModules ? JSON.stringify(modulesExported[_this.id]) : cssVariableName;
        output += `var ${cssVariableName} = ${JSON.stringify(result.css)};\n` + `var styleMapping = ${module};\n` + `export var stylesheet=${JSON.stringify(result.css)};`;
}

if (!shouldExtract && shouldInject) {
    output += typeof options.inject === 'function' ? options.inject(cssVariableName, _this.id) : '\n' + `import styleInject from '${styleInjectPath}';\n` + `export default /*#__PURE__*/(function() {\n  styleInject(${cssVariableName}${Object.keys(options.inject).length > 0 ? `,${JSON.stringify(options.inject)}` : ''});\n  return styleMapping;\n})();`;
}

For others looking at this comment, I also added a feature request to a similar plugin: Anidetrix/rollup-plugin-styles#168

Thank you so much for that! I created a pull request, let's hope they merge it.

ch99q avatar Mar 17 '22 17:03 ch99q