Tree Shaking with sideEffects false

[daaku]

I'm trying to diagnose why this package:
https://www.npmjs.com/package/@primer/octicons-react

Which contains the required magical incantation of sideEffects: false:
https://github.com/primer/octicons/blob/main/lib/octicons_react/package.json#L10

With usage like this:

import React from 'react';
import ReactDOM from 'react-dom';
import { XIcon } from '@primer/octicons-react'

ReactDOM.render(<XIcon />, document.getElementById('root'));

Ends up with all the exports from @primer/octicons-react in the bundle. I'm guessing it's because of the assignments to defaultProps, but shouldn't sideEffects: false trigger ignoring those?

Sample repository here: https://github.com/daaku/esbuild-octicons-shake-test

[evanw]

AFAIK sideEffects: false only means "this file can be removed if none of the imports of this file are used." It does not say anything about the statements in the file, which can still have side effects and can't necessarily be removed. I'm mainly going by this previous discussion since I don't see any information that says one way or the other in Webpack's documentation about this feature.

I agree that your interpretation sounds reasonable. This is also along the lines of what I was thinking in that past thread here:

I could see an alternative algorithm where, if "sideEffects": false is present, code with side-effects can form a connected component in the top-level statement graph but connected components could still be dropped if there are no references into the connected component from the outside (also similar to garbage collection). But referencing something in the connected component would pull in the whole connected component. So referencing foo would also pull in the assignment to foo.bar.

However, I'm not aware of any other bundler that interprets the sideEffects field that way, so doing that sounds potentially dangerous since it would mean esbuild would be behaving differently than other bundlers and could potentially remove code with side effects that is important to retain. The conservative thing to do is to do nothing which is what I have done so far.

[daaku]

Sounds like this is similar to the displayName case for react-icons from your tree-shaking-example repository based on the issue you linked to.

What unsafe assumption do rollup, parcel, webpack make that makes them ignore that assignment. Is it that property assignments are considered side-effect free? Would you be willing to optionally enable such an assumption? As seen from your example, this affects the bundle size for icon libraries, component libraries etc (often?). I have another case besides this one with @primer/components.

Is there already a way to leverage Pure to mark these assignments as pure as a consumer of such a library?

[daaku]

However, I'm not aware of any other bundler that interprets the sideEffects field that way, so doing that sounds potentially dangerous since it would mean esbuild would be behaving differently than other bundlers and could potentially remove code with side effects that is important to retain.

My 2c: Rather than implementing a safer-but-not-the-norm version of how sideEffects are considered, which I'm guessing is actually harder to build, I would suggest building the unsafe-but-the-norm version of how sideEffects are already considered by other bundlers (wrt property assignment, assuming my understanding is correct).

[evanw]

I would suggest building the unsafe-but-the-norm version of how sideEffects are already considered by other bundlers (wrt property assignment, assuming my understanding is correct).

The problem with this is that from what I understand, it's extremely tricky to build and would be a big step up in complexity for esbuild's tree shaking implementation. You basically have to model the evolution of the object shape as it's constructed and track all references to the object to determine whether or not an assignment has side effects.

By comparison, the connected component approach would be simple to implement and (at least to me) seems like it would be relatively robust. It would only work under the assumption that property assignments don't have side effects given sideEffects: false, since property assignments generally have side effects. I'd prefer to implement this simpler approach instead of the more complex approach. I'd need to think more about this approach though to come up with a more formal definition of the optimization and check for edge cases.

From the library authoring end, the most straightforward way to allow code with possible side effects to be tree shaken within a single file with the current version of esbuild is to use a pure annotation like this:

export var ChecklistIcon = /* @__PURE__ */ (() => {
  function ChecklistIcon(props) {}
  ChecklistIcon.defaultProps = {}
  return ChecklistIcon
})()

This is something that has to be done on the library authoring end, however. It cannot be done on the library consumption end.

[daaku]

The problem with this is that from what I understand, it's extremely tricky to build and would be a big step up in complexity for esbuild's tree shaking implementation. You basically have to model the evolution of the object shape as it's constructed and track all references to the object to determine whether or not an assignment has side effects.

That does seem significantly more complex. It's kinda crazy that we have to rely on this for dead code elimination!

In an ideal world we'd push library authors to split code into little ESMs. Icon and Component libraries should for sure be doing this.

Thanks for your efforts!

[kzc]

You basically have to model the evolution of the object shape as it's constructed and track all references to the object to determine whether or not an assignment has side effects.

That does seem significantly more complex. It's kinda crazy that we have to rely on this for dead code elimination!

Dropping objects/functions/classes without consideration to all their references would lead to incorrect results.

Pure annotated calls were invented by uglify and embraced by babel and webpack to get around the inability at the time to eliminate unused class definitions that were down-leveled to ES5. Uglify has since improved its optimization to automatically handle many of these cases without annotations, but the feature is still useful to drop calls with unused return values - whether the calls contain side effects or not.

As for the webpack "sideEffects": false convention, keep in mind that webpack historically relied on uglify (and now a fork, terser) to solely perform its dead code elimination. This was in the days prior to the popularization of scope hoisting by Rollup, when webpack put each module into a separate function closure as browserify did. So when webpack introduced the package.json sideEffects feature it was an all or nothing proposition whether to include certain source files, which explains their implementation's current binary behavior.

[rtsao]

It appears webpack will indeed prune "unused" statements from side effect-free files, even statements with obvious side effects such as console.log.

I've created a simple repo to demonstrate this: https://github.com/rtsao/side-effects-statements-test

Obviously there is not a clear specification for the sideEffects field, but I think esbuild is actually deviating from de-facto spec (webpack) in this regard by not tree shaking statements. My understanding is that sideEffects: false has always meant that nothing in the file should have effects when imported, and thus even individual statements can be safely pruned assuming they aren't imported or referenced by what is being imported.

I'm attempting to migrate some apps from webpack to esbuild and this difference in behavior is a bit of a hurdle at the moment.

[kzc]

Here's a simplified version of @rtsao's example. It shows the unwanted retention of side effect code within non-entry-point sideEffects: false modules that do not contain any locally defined exported items that are actually used by the final bundle.

Although webpack is not shown here, rollup and webpack produce the same result for this example. Rollup is just easier to use for illustrative purposes.

// package.json
{ "sideEffects": false }

// index.js
console.log("__index__");                          // retain all side effects in entry point
import {A, Unused1, Unused2} from "./dep.js";
A();                                               // retain all side effects in entry point

// dep.js
export {A, Unused2} from "./a.js";                 // pass through - no local defs used
console.log("__dep__");                            // <--- should be dropped
export const Unused1 = console.log("__effect1__"); // <--- should be dropped

// a.js
export const A = () => console.log("__A__");       // local def used in final bundle
console.log("__a__");                              // retain all side effects in module
export const Unused2 = console.log("__effect2__"); // retain all side effects in module

$ rollup index.js -p node-resolve --silent | node
__a__
__effect2__
__index__
__A__

$ esbuild index.js --bundle | node
__a__
__effect2__
__dep__
__effect1__
__index__
__A__

Consider dep.js to be a sideEffects: false "pass through" module. All side effect code in such modules can be dropped.

Note: the example was revised to demonstrate the "all or nothing" nature of side effect retention in non-entry-point sideEffects: false modules.

[daaku]

Would be great to know how you see the priority of this - not trying to push, just trying to understand so I can decide how to work around it. I would try to contribute, but understanding the tree shaking logic seems more work than I'd like to sign up for right now ;)

[fabiospampinato]

This not working is kind of a big deal for me, it causes esbuild to produce larger bundles, or even outright fail to bundle, for example when importing an isomorphic export out of a module that also has some Node-only exports.

[fabiospampinato]

Also both webpack and rollup tree-shake unused exports from packages having sideEffects: false, I'm not aware of any other bundler that works like esbuild in this regard.

[imranbarbhuiya]

I have an esbuild plugin for node modules polyfill. And I'm also having a similar issue. When a user imports a single fn from a module, I need to give the full contents of the module to esbuild and esbuild doesn't tree shake the code so full module gets added in the final bundle. In this pr, only gzip was getting used from zlib module but esbuild is adding complete zlib module code making the bundle size too big