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.

Sign up for GitHub

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

fix: Enabling sideEffects on style loaders #1423

Merged
merged 7 commits into from
Mar 9, 2021
+51 −0
Merged

fix: Enabling sideEffects on style loaders #1423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
30a2e15
fix: Enabling sideEffects for style loaders
rschristian Sep 14, 2020
7d80cb6
refactor: Don't need to enable sideEffects with modules
rschristian Sep 15, 2020
1097cf3
chore(ignore): Excluding new `size-plugin-ssr`
rschristian Sep 15, 2020
a9eb285
test: Adding test to ensure side effects ignored for non-modules css …
rschristian Sep 15, 2020
bd88168
refactor: Adding comment to explain
rschristian Sep 15, 2020
9ecd194
fix(changeset): Adding changeset
rschristian Sep 23, 2020
4dc6675
refactor: Removing `size-plugin-ssr.json` from gitignore
rschristian Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/gold-pumas-invite.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 5 additions & 0 deletions packages/cli/lib/lib/webpack/webpack-base-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +272 to +275
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment taken right from CRA, I felt that having an explanation in the source is useful.

sideEffects: true,
},
{
test: /\.(xml|html|txt|md)$/,
Expand Down
10 changes: 10 additions & 0 deletions packages/cli/tests/build.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 17 additions & 0 deletions packages/cli/tests/images/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,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 = {};
Expand Down
6 changes: 6 additions & 0 deletions packages/cli/tests/subjects/side-effect-css/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { h } from 'preact';
import './style.css';

export default () => {
return <h1>SideEffect CSS test</h1>;
};
5 changes: 5 additions & 0 deletions packages/cli/tests/subjects/side-effect-css/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"private": true,
"name": "side-effect-css",
"sideEffects": false
}
3 changes: 3 additions & 0 deletions packages/cli/tests/subjects/side-effect-css/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
h1 {
background: #673ab8;
}