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

styleInject breaks tree-shaking #293

Open
adi518 opened this issue Jul 16, 2020 · 12 comments
Open

styleInject breaks tree-shaking #293

adi518 opened this issue Jul 16, 2020 · 12 comments
Labels

Comments

@adi518
Copy link

adi518 commented Jul 16, 2020

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).

@wardpeet
Copy link
Collaborator

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.

@adi518
Copy link
Author

adi518 commented Aug 31, 2020

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

@benmccann
Copy link
Contributor

benmccann commented Sep 5, 2020

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

@adi518
Copy link
Author

adi518 commented Sep 5, 2020

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.

@benmccann
Copy link
Contributor

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

@dapetcu21
Copy link

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?

@adi518
Copy link
Author

adi518 commented Sep 5, 2020

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.

@andkh
Copy link

andkh commented Oct 28, 2020

Any updates?

@adi518
Copy link
Author

adi518 commented Oct 28, 2020

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

@danlevy1
Copy link

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

@danlevy1
Copy link

danlevy1 commented Mar 3, 2021

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

ch99q added a commit to ch99q/rollup-plugin-postcss-fork that referenced this issue Mar 17, 2022
ch99q added a commit to ch99q/rollup-plugin-postcss-fork that referenced this issue Mar 17, 2022
@ch99q
Copy link

ch99q commented Mar 17, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7 participants