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 <[email protected]> Co-authored-by: Ryan Christian <[email protected]>
preactjs · Mar 8, 2021 · e1033b7 · e1033b7
1 parent c33f020
commit e1033b7
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 0 deletions.
diff --git a/.changeset/gold-pumas-invite.md 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
@@ -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
@@ -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
@@ -53,6 +53,23 @@ exports.sass = `
 </body>
 `;
 
+exports.sideEffectCss = `
+<head>
+  <meta charset="utf-8">
+  <title>side-effect-css<\\/title>
+  <meta name="viewport" content="width=device-width,initial-scale=1">
+  <meta name="mobile-web-app-capable" content="yes">
+  <meta name="apple-mobile-web-app-capable" content="yes">
+  <link rel="apple-touch-icon" href=\\"\\/assets\\/icons\\/apple-touch-icon\\.png\\">
+  <link rel="manifest" href="\\/manifest\\.json">
+  <style>h1{background:#673ab8}<\\/style>
+  <link href=\\"/bundle.\\w{5}.css\\" rel=\\"stylesheet\\" media=\\"only x\\" onload=\\"this.media='all'\\">
+	<noscript>
+    <link rel=\\"stylesheet\\" href=\\"\\/bundle.\\w{5}.css\\">
+  </noscript>
+<\\/head>
+`;
+
 exports.prerender = {};
 
 exports.prerender.heads = {};

diff --git a/packages/cli/tests/subjects/side-effect-css/index.js b/packages/cli/tests/subjects/side-effect-css/index.js
@@ -0,0 +1,6 @@
+import { h } from 'preact';
+import './style.css';
+
+export default () => {
+	return <h1>SideEffect CSS test</h1>;
+};
diff --git a/packages/cli/tests/subjects/side-effect-css/package.json b/packages/cli/tests/subjects/side-effect-css/package.json
@@ -0,0 +1,5 @@
+{
+  "private": true,
+  "name": "side-effect-css",
+  "sideEffects": false
+}
diff --git a/packages/cli/tests/subjects/side-effect-css/style.css b/packages/cli/tests/subjects/side-effect-css/style.css
@@ -0,0 +1,3 @@
+h1 {
+	background: #673ab8;
+}