From e1033b715283fbb523ab373dbae58ed6844734c5 Mon Sep 17 00:00:00 2001
From: Ryan Christian
Date: Mon, 8 Mar 2021 23:45:32 +0000
Subject: [PATCH] fix: Enabling sideEffects on style loaders (#1423)
**What kind of change does this PR introduce?**
Bugfix / feature
**Did you add tests for your changes?**
Yes. Added a test to ensure that non-module CSS imported within a package that has `"sideEffects": false` would result in CSS being inlined in the head of the document. If the change to the style loader is reverted, the CSS import is ignored (skipped? not sure what it technically does) and the CSS will not end up being inlined.
**Summary**
Fixes #1411
I noticed this issue while building a component library. This library uses the Preact CLI to (mainly) run a development environment and Microbundle is used to the build/package the library itself. This is all well and fine.
The issue that this PR fixes arose when I had decided that my dev environment looked good enough to toss up as demo of the library. The `sideEffects` field stops the Preact CLI from importing non-module CSS when set to `false` in a production build. This should not happen as `import 'x.css';` will always have a side effect. It's the very nature of that type of import.
Reading through various threads led me to CRA's solution, which was to [enable sideEffects for their style loaders](https://github.com/facebook/create-react-app/blob/65d8eb23f28475716e7e9497215cd574c1bf0b6c/packages/react-scripts/config/webpack.config.js#L502). Doing something similar with our setup seems to work just as well.
@developit had suggested something along the lines of the following which also would work with slight modification:
```
// in webpack.config.base.js
module: {
rules: [
{
test: /\.m?[jt]sx?$/,
exclude: nodeModules,
sideEffects: true
}
]
}
```
Changing this to target CSS works just as well as what I did in this PR. I figured it's simpler to just add a field to the existing loader rather than add a new one. There might be a distinction between the two methods, but I'm not sure what that would be.
**Does this PR introduce a breaking change?**
Shouldn't be breaking.
Co-authored-by: Ryan Christian <33403762+RyanChristian4427@users.noreply.github.com>
Co-authored-by: Ryan Christian <33403762+rschristian@users.noreply.github.com>
---
.changeset/gold-pumas-invite.md | 5 +++++
.../cli/lib/lib/webpack/webpack-base-config.js | 5 +++++
packages/cli/tests/build.test.js | 10 ++++++++++
packages/cli/tests/images/build.js | 17 +++++++++++++++++
.../cli/tests/subjects/side-effect-css/index.js | 6 ++++++
.../tests/subjects/side-effect-css/package.json | 5 +++++
.../tests/subjects/side-effect-css/style.css | 3 +++
7 files changed, 51 insertions(+)
create mode 100644 .changeset/gold-pumas-invite.md
create mode 100644 packages/cli/tests/subjects/side-effect-css/index.js
create mode 100644 packages/cli/tests/subjects/side-effect-css/package.json
create mode 100644 packages/cli/tests/subjects/side-effect-css/style.css
diff --git a/.changeset/gold-pumas-invite.md b/.changeset/gold-pumas-invite.md
new file mode 100644
index 000000000..9aa030568
--- /dev/null
+++ b/.changeset/gold-pumas-invite.md
@@ -0,0 +1,5 @@
+---
+'preact-cli': patch
+---
+
+Fixes bug with style loader that would strip non-module CSS files if 'sideEffects' was set to false for the package.
diff --git a/packages/cli/lib/lib/webpack/webpack-base-config.js b/packages/cli/lib/lib/webpack/webpack-base-config.js
index 1b6923ffb..4cda50a71 100644
--- a/packages/cli/lib/lib/webpack/webpack-base-config.js
+++ b/packages/cli/lib/lib/webpack/webpack-base-config.js
@@ -269,6 +269,11 @@ module.exports = function (env) {
},
},
],
+ // Don't consider CSS imports dead code even if the
+ // containing package claims to have no side effects.
+ // Remove this when webpack adds a warning or an error for this.
+ // See https://github.com/webpack/webpack/issues/6571
+ sideEffects: true,
},
{
test: /\.(xml|html|txt|md)$/,
diff --git a/packages/cli/tests/build.test.js b/packages/cli/tests/build.test.js
index a30463776..9235007e6 100644
--- a/packages/cli/tests/build.test.js
+++ b/packages/cli/tests/build.test.js
@@ -208,6 +208,16 @@ describe('preact build', () => {
expect(() => build(dir)).not.toThrow();
});
+ it('should import non-modules CSS even when side effects are false', async () => {
+ let dir = await subject('side-effect-css');
+ await build(dir);
+
+ let head = await getHead(dir);
+ expect(head).toEqual(
+ expect.stringMatching(getRegExpFromMarkup(images.sideEffectCss))
+ );
+ });
+
it('should copy resources from static to build directory', async () => {
let dir = await subject('static-root');
await build(dir);
diff --git a/packages/cli/tests/images/build.js b/packages/cli/tests/images/build.js
index 617c0cde1..5cf59dc6b 100644
--- a/packages/cli/tests/images/build.js
+++ b/packages/cli/tests/images/build.js
@@ -53,6 +53,23 @@ exports.sass = `
`;
+exports.sideEffectCss = `
+