Effectful imports should ignore sideEffects field

[Andarist]

Do you want to request a feature or report a bug?

feature

What is the current behavior?

import 'list/fantasy-land'

with

sideEffects: false

does not include imported file in the bundle, which is somewhat OK

If the current behavior is a bug, please provide the steps to reproduce.

not a bug

What is the expected behavior?

this import is clearly effectful, so it actually IMHO should not get dropped regardless of the sideEffects

If this is a feature request, what is motivation or use case for changing the behavior?

described in expected behaviour

Please mention other relevant information such as the browser version, Node.js version, webpack version, and Operating System.

webpack@4, others - irrelevant

[sokra]

If import 'list/fantasy-land' has a side-effect, you sideEffects configuration is wrong.

You can and should include list/fantasy-land in your configuration like this:

// package.json of 'list'
{
  "sideEffects": [
    "./fantasy-land"
  ]
}

[Andarist]

I understand how to solve it, I've did it just before posting an issue here and I understand this is not the most popular case, but still think that my argument has some merit - import 'package' is side-effectful "by definition" and sideEffects could skip such imports by default.

[quantizor]

I agree with @Andarist. If you consider the import statement, a naked import implies “execute everything in this file”, which is side effectful by default.

[dcporter]

I also agree with @Andarist. MDN describes import '/modules/my-module.js'; as "Import a module for its side effects only", and such imports have no other purpose. The use of this form of import syntax is sufficient information to indicate that the imported module has side effects.

So the developer has already specified that this module has side effects; the requirement to specify it again in a separate location is suboptimal and fragile.

[rtsao]

In practice, it might be nice to configure webpack to assume all modules are side effect free by default, as the whole npm ecosystem doesn't use it yet.

If webpack treated an import statement with no specifiers (i.e. import "foo") as a signal for side effects, this would be a convenient, zero-config way to opt out of this behavior (as opposed to #6074).

For example:

import {something} from "foo";

// unused import gets pruned

import "foo";
// implies side effects

import {something} from "foo";

// unused import does not get pruned

This behavior could be nice combination with:

• Warnings when imports are pruned but not explicitly "sideEffects": false
• Warnings when imports are explicitly "sideEffects": false but they have been imported without specifiers (i.e. import "foo";)

[edmorley]

Hi

In facebook/create-react-app#5140 the package in question mistakenly set "sideEffects": false even though it includes CSS, which caused the CSS to be dropped when mode was 'production'.

This lead to the discussion here:
facebook/create-react-app#5141 (comment)

And similar confusion occurred in #6741 and #7791.

@sokra, what do you think about the idea of allowing loaders to declare that their output always has side effects, thereby overriding the package.json sideEffects (perhaps emitting a warning whilst doing so)? css-loader could then use this to declare that its output should never be optimized away by SideEffectsFlagPlugin.

[gaearon]

I disagree that the import form should determine dead code elimination. That sounds like a dangerous heuristic to me because it’s so subtle. I think it’s fair that the package author should declare effectful entry points separately. However I also think webpack could warn when you use such an entry point with a plain import.

That said the way webpack handles CSS in this case doesn’t make sense to me. See facebook/create-react-app#5141 (comment). When you import CSS package library can’t “know” how you’ll “interpret” it. So it can’t know whether it will become a side effect or not. The real effect here is in the style loader. So that leads me to thinking loaders should be able to mark their output as effectful.

[rtsao]

It would be nice if the entire npm ecosystem adopted the sideEffects package.json field, but realistically, I think it will be a long time before that is the case.

Consider the following scenario:

• The top-level configuration of webpack has been set tosideEffects: false.
• Package "core-js" is explicitly imported for side effects (i.e. import "core-js";) and omits a sideEffects field in its package.json

In this scenario, I think pruning "core-js" is clearly incorrect behavior. I certainly don't think import form should be the only heuristic for determining dead code elimination, but in this specific case where a package's side effectfulness is not explicitly specified, not using that information seems just plain wrong.

Until a majority of the npm ecosystem specifies a sideEffects field, it would be nice to allow users to have a default behavior of pruning unused imports (which corresponds to the vast majority of packages) but allow unambiguously effectful imports (import "foo";) to be preserved without extra configuration.

[gaearon]

@rtsao I think in the scenario you're describing treating such imports specially only adds to the overall confusion. Because now some cases will work and some wouldn't, but the problem will get less widespread and thus there'll be less incentives to properly fix it. So it could cause more pain in longer term.

Your solution is probably reasonable for the behavior when the user explicitly sets top-level sideEffects to false (which I didn't even know is possible — how can the user know everything about their dependencies? this sounds like a dangerous option that most people will misunderstand).

But for cases when you don't have anything on the top level, I don't think plain import should be treated as effectful in contradiction to package's own explicit declaration. For those cases IMO webpack should warn but still respect the package declaration. Otherwise it's too unpredictable.

[dcporter]

The import “foo” form is an extremely clear indication of developer intent. There is no case in which one uses that import form and doesn’t want it included (assuming the importing file is included) — that’s what that form is for, and it’s the only thing it’s for. That clear, language-level developer declaration should in all cases override the sideEffects flag, and not just for now—broad, clear understanding and use of the flag won’t change the clarity of developer intent when importing a module without symbol.

[gaearon]

The reason I'm opposed to this is because it's unexpected to see refactoring from

import 'foo'

to

import {x} from 'foo'

break code.

For example, somebody might be running some code just for side effects, relying on your proposed behavior. Then they actually need to add an import, and it's super easy to miss that adding an import would remove the side effects in production only.

Of course you can have two import statements but then that is unexpected too.

import 'foo' 
import {x} from 'foo'

Anybody can refactor this to "remove duplicate import" (in fact there's probably linter plugins to do that or complain about that). It's too subtle for this to change the behavior.

We need to consider not just how the code looks, but how people tend to change it over time, and anticipate the pitfalls.

This is why I think webpack should warn when you use plain import (correctly guessing your intent, and surfacing the problem to you). But I don't think it should change the behavior. Because the fix should be in the package that "lies" about its declared side effects. Warning surfaces the problem, package author fixes it, and everybody is happy.

[sokra]

A warning is definitely a good idea.

Using import "x" when x is sideeffect-free seems to be wrong to me, either x isn't really sideeffect-free and the package.json is incorrect or the import should be removed because it has no effect. So accepting import "x" as forced inclusion doesn't seem to be a valid option to me.

[sokra]

Some note about adding sideEffects: false to your application. This will not inherit to the node_modules. Only the nearest package.json in the tree is used to read the flag. -> flag affects only the package and not dependencies

[sokra]

Regarding importing CSS. While the exact way how CSS is handled is maybe not defined, the semantic of import "file.css" is defined. It means: "the rules in the file become somehow effective". And the semantic is also the thing that defines if it has sideeffects, which is has. So a module can flag CSS as sideeffect-full even if it doesn't know how it's handled exactly.

Anyway, you can also set sideEffects in the CSS rule in module.rules.

[gaearon]

Thanks, this makes sense!

[gaearon]

Anyway, you can also set sideEffects in the CSS rule in module.rules.

Do you feel it would be a reasonable default though? We need to decide what CRA should do in facebook/create-react-app#5140.

[dcporter]

Using import "x" when x is sideeffect-free seems to be wrong to me

If the developer wrote import “x”, the developer said to include it for its side effects, full stop. Webpack shouldn’t be thinking any more about it than that. (Again, assuming that the importing file is included.) If the developer got that wrong, that’s fine, they get an extra file that they explicitly asked for. Again, accepting it as forced inclusion is what that form exists for, and is the only thing it exists for.

it's unexpected to see refactoring from

import 'foo'

to

import {x} from 'foo'

break code.

I agree that’s a surprising result, and I agree that having two imports is gross (and probably lint-breaking). But the refactored code is incorrect (and is therefore not a refactor): importing x and not using it is a mistake, and isn’t the same as importing foo for side effects. I’d also expect it to be a rarely-sprung trap: most side effect files (e.g. CSS files) have no exports. So breaking a language-level intuition to protect developers from the results of a rare, double mistake, feels like a pretty bad trade-off to me.

Anecdotally, every coworker whose opinion I have initially expects side-effect imports to work with sideEffects: false. (Similarly unscientific: my post above has ten 👍s.) The idea that the side-effect import should work is a simple language-level intuition, which is correct inasmuch as it’s backed up by MDN. sideEffects: false should honor it, and can do so without being less useful or powerful.

[sokra]

Do you feel it would be a reasonable default though?

Sideeffect-full is the default. You opt-in into sideeffect-free with the package.json-flag.

Being always sideeffect-full for CSS feels like not respecting the opt-in in the package.json and promoting an incorrect sideEffects config in package.json.

Note that CSS modules can be flagged as sideeffect-free.

[sokra]

If the developer wrote import “x”, the developer said to include it for its side effects, full stop.

If the developer wrote "sideEffects": false, the developer said that it has no side effects, full stop.

These are two conflicting statements the developer made here. Maybe we should even emit an error here.

[dcporter]

Anecdotally, every coworker whose opinion I have initially expects side-effect imports to work with sideEffects: false.

(These are folks who get what the flag is for and think it’s awesome, by the way.)

[dcporter]

These are two conflicting statements the developer made here. Maybe we should even emit an error here.

They are not: they’re intuitively compatible (“put me in no-side-effects mode and let me clearly declare side effects the usual JS-y way”), and could be made actually compatible if you slightly change the semantics of sideEffects: false to match this common intuition. This change would not dilute the meaning or utility of this excellent flag.

I would definitely support adding an error over the status quo of silently stripping them against the developer’s clear intentions. But it would be a really silly error. “We know what you meant here, you couldn’t have meant anything else, but error.”

[gaearon]

While the exact way how CSS is handled is maybe not defined, the semantic of import "file.css" is defined. It means: "the rules in the file become somehow effective". And the semantic is also the thing that defines if it has sideeffects, which is has. So a module can flag CSS as sideeffect-full even if it doesn't know how it's handled exactly.

Based on this, it seems like webpack could always error if you import CSS from package that says it has no side effects? Since it's never correct.

[gaearon]

In Create React App, we've decided to always treat css/sass/scss as side effectful in the module rules. If webpack adds a warning (or better, an error) when CSS is marked as side effect-less, we can remove this customization. But for now it's too subtle and would bite people in production otherwise.

[jasonk]

FWIW, I agree with the sentiment that import "./file"; should work this way, largely because while the sideEffects flag is useful, it's also terribly confusing.

For example here are snippets of some of the files from the project where I ran into this issue:

// entry.js
import './icons';
// ..bunch of other irrelevant imports

// icons/index.js
import './library';
export * from './Icon';

// icons/library.js
import { library } from '@fortawesome/fontawesome-svg-core';
import * as icons from './icon-registry';
library.add( ...Object.values( icons ) );

// icons/icon-registry.js
export {
  faUserCircle as farUserCircle,
  faCircle as farCircle,
  faIdCard as farIdCard,
} from '@fortawesome/free-regular-svg-icons';

The problem that I had is that when running webpack --mode=production none of the icons made it into the bundle, but it worked fine when running with mode=development. I had read through the tree-shaking docs and knew about sideEffects so I added the module that had side effects to my package.json:

"sideEffects": [ "src/icons/library.js" ],

This didn't do anything. After some experimentation I arrived at:

"sideEffects": [ "src/icons/library.js", "src/icons/index.js" ],

So basically it seems that part of the issue with sideEffects is that while you clearly have to include modules that have actual side-effects, in some situations you also need to include modules that simply import things that have side effects. Or maybe the problem is that "side effects" is not well-defined. In either case, having import "./foo" be treated as having side effects would avoid this confusion (and given the number of posts I found from people having problems with this, it seems to be a fairly common source of confusion).

[dcporter]

I agree wholeheartedly with the principle of least astonishment. I'm astonished that anyone considers "imports should work the way that the JavaScript language says they should work " more astonishing than "a package.json flag that webpack made up a year and a half ago should override the language".

In a practical use, projects may be using overrides that should only be effective in dev mode

I'm not clear what you're describing here, does sideEffects behave differently in prod than in dev? That's terrifying. Changing import behavior between dev and prod is a deeply hacky, wrong-layer use of this package.json feature. That sort of behavior should live in webpack configs, and should definitely not live in package.json. import behavior should absolutely not implicitly change between dev and prod like you describe, that's a massive trap.

Adding implicit deductions in this decision process, as proposed by this feature request, does not scale out well

JavaScript has a clear language-level way to declare side-effectful imports. This proposal doesn't suggest adding anything implicit: it asks that this flag from a build tool not override the language. Developers know what import means, and have never heard of sideEffects until they're deep into webpack academia.

I would ask that the webpack team considers what's expected, surprising, and explicit from the perspective of developers using the JavaScript language itself. Every single time we've used this feature with new teams, developers are astonished that their clear-as-day side-effectful imports (import "./path/to/file.css") get stripped.

(sidebar: <3 your avatar.)

[chybisov]

Any updates on this?

[geoffp]

Just found this because I am astonished that Webpack is throwing away all my hard work on a polyfill loader. :)

FWIW, I also expected that import "./sideEffects"; would not be tree-shaken out by default, since that is, semantically, an explicit import of something that can only have side effects. I've gotten it working now, but only after a long struggle.

The documentation on sideEffects could probably use a little detail on what's necessary for library authors, and also make clear that you have to list "import ancestors" of side-effecty files you want to keep. I found that I had to both list ancestors, and make the paths point at build artifacts (in ./dist/, in my case) rather than source files. Like so:

"sideEffects": [
  "./dist/polyfills/index.js",
  "./dist/index.js"
]

(Assuming that index.js imports ./polyfills). The first line on its own appeared not to be enough, as others have mentioned.

[strass]

I agree that this is a cruel trap and that it should at least be consistent between dev and prod

[scamden]

@dcporter

I don't believe webpack does sub-module tree shaking,

Are you sure about that? Perhaps it is due to two different optimizations (usedExports vs sideEffects) but if I add an exported function to a module and don't import it webpack definitely drops it.

[sokra]

I agree that this is a cruel trap and that it should at least be consistent between dev and prod

In webpack 5 the side effects optimization is enabled for both modes to reduce differences.

[scamden]

@sokra do the marked side effects also get dropped even without minification?

[sokra]

Yes

[ghost]

Wait, so was this feature actually implemented? Is it safe to use:

import 'file.css'
// or
import 'file.js'

// With { sideEffects: false } in package.json?
// -------------
// Will the contents of file.js appear in the bundle?

[Andarist]

Will the contents of file.js appear in the bundle?

No, since you have configured sideEffects: false and no import from that file ends up being used.

[benjamingr]

Like @gaearon this seems like it should be up to the loader or plugins shouldn't it?

Is there a way to explicitly mark a module as "having side effects" from a loader/plugin currently?

Edit: My (awful) solution was to write a ts transform (or a loader if not using TS) to rewrite these import statements and use them in a way that webpack doesn't understand which is a pretty bad hack.

[alfaproject]

I also agree with @Andarist and was a bit surprised by this today when I decided to finally enable the side effects flag for our monorepo. Tree shaking is way better but alas it also dropped the naked imports which I thought would be enough of a flag for webpack to leave them alone by default )':