-
-
Notifications
You must be signed in to change notification settings - Fork 215
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
Comments
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. |
Sure, I have a repro in hand. Will post it here soon. 👍 |
I'm guessing you're referring to tree-shaking in webpack specifically? |
Tree-shaking in every tree-shaking compatible tool (Webpack, Rollup, etc'). The issue here is that |
Ah, I didn't know Rollup supported these annotations, but it looks like it does: https://rollupjs.org/guide/en/#treeshake |
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? |
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. |
Any updates? |
Hi, no. I was busy with other stuff. I intend to come back to this though, because I really need to solve this issue. |
Just pinging this thread to see if there is any progress. This would be so great! |
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
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 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. |
This function doesn't have a
/*#__PURE__*/
prefix, which breaks tree-shaking. Moreover, when I try to prepend it myself throughinject
option it gets stripped along with the entire string from the final bundle, which leaves you without CSS.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).The text was updated successfully, but these errors were encountered: