Tree-shaking rxjs 6 produces left-over non-exported functions

[43081j]

Given the following:

import { Subject } from 'rxjs';
import { take } from 'rxjs/operators';
export function foo() {
    const s = new Subject();
    s.next('foo');
    return s.pipe(take(1));
}

And config:

import resolve from 'rollup-plugin-node-resolve';

export default {
  input: 'lib/index.js',
  output: {
    file: 'bundle.js',
    format: 'iife',
    name: 'foo'
  },
  plugins: [
    resolve()
  ]
};

The rxjs/operators import resolves to rxjs/_esm5/operators/index.js which looks something like:

export { audit } from './audit';
export { auditTime } from './auditTime';
export { buffer } from './buffer';
// all operators...

We end up with a 214K bundle, for 9 lines of code and two imports.

---

Looking at the bundle, it seems unexported functions/classes are somehow kept.

Take this operator for example.

In our bundle:

• The exported functions from this operator are indeed shaken (as we don't use them and our imports don't seem to)
• The classes which are not exported from this operator are in the resulting bundle (such as this class)

---

What is going on here? Is this rx's fault or rollup's or mine?

How do we possibly end up with classes in our bundle which were never even exported?

---

Edit

rxjs depends on tslib which seems to be the culprit for marking everything as having side effects

I stepped through rollup's traversal for a while to see which node returned true for hasEffects. It turns out to be this helper from TypeScript's tslib helpers.

The CallExpression here goes through the usual checks (hasEffects):

CallExpression -> callee (Identifier) -> variable (LocalVariable)

Where the LocalVariable is this. Because isReassigned is true on that, everything which uses this helper ends up being included.

Any idea how to combat this? This is depended on by rx, not my code. Is there a way maybe to let rollup know we don't care about it?

[lukastaegert]

Thanks for digging into this. TypeScript helpers are indeed nasty as they rely on mutations a lot which our analysis just gives up on as you have noticed (with the isReassigned flag). There are existing (longer term, we are talking months here) plans to refactor our logic to be able to handle mutations which will have the potential to finally solve this.

As you already did some digging, if you could put the gist of your findings into a REPL link that contains the helper code in question and how it is used in an example where you would expect tree-shaking, this would be a huge help as this could give us an important real-world ready-to-use test-case once we implement the new logic.

[43081j]

Try this example.

Out of interest, what kind of "new logic" would solve this problem?

Something like traversing further into the variable declarator and checking what it really modifies?

[lukastaegert]

Thanks a lot!

The rough issue is that if a variable has been marked as "reassigned", the current logic becomes very conservative about further modifications. The new approach would be akin to calculating a "single static assignment form", which means modifications are treated as definitions of new variables with different values. We would then run the tree-shaking on the new form to determine which modifications do not have effects because their resulting variables are not used.

However with your example, we can figure out if there are ways to extend the existing logic as well, which could provide a solution in a shorter frame of time.

[Buslowicz]

I've noticed something similar when importing import { Observable } from './node_modules/@reactivex/rxjs/dist/esm2015/index.js'; . The bundle contains not just Observable and its dependencies, but seems to also include everything that uses Observable or Subscriber (dependency of Observable). This results in around twice as big bundle (in my very minimal case).
Could it be the same issue, or is it something different? I can create a repro GH if needed.

[Buslowicz]

Hello again. @lukastaegert when can we expect the new algorithm to be implemented and thus this issue fixed? I am starting a new project and am facing a bundler choice decision. If this would be fixed in the next 2-3 months I would definitely stay with rollup (I am a fan ^^), but if not, I will have to use something I like less.

[kzc]

@lukastaegert Does Rollup support the Webpack package.json convention for module sideEffects? This coarse grained hint would be pertinent to this issue. I couldn't find any reference to it in the source or documentation for Rollup, rollup-plugin-node-resolve or rollup-plugin-commonjs.

• https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free
• https://github.com/webpack/webpack/tree/master/examples/side-effects
• ReactiveX/rxjs@47351d6
• https://github.com/openlayers/openlayers/blob/a73e9607a8f379d8d9383c060705111f5f6c81e8/package.json#L101-L103

The effectiveness of sideEffects is particularly evident in this example:

• https://github.com/mischnic/tree-shaking-example
• https://github.com/mischnic/tree-shaking-example/blob/master/src/lodash-es.js

parcel/lodash-es    19.28 KB
rollup/lodash-es    81.08 KB
webpack/lodash-es   20.74 KB

Webpack and Parcel take advantage of this package hint.

[lukastaegert]

As rollup does not read package.json files, this would be needed to be implemented somehow in rollup-plugin-node-resolve but there is no API for this in rollup yet.

As for the algorithmic rewrite, 2-3 months will probably not be enough but adding support for sideEffects should be doable in that time-frame.

[lukastaegert]

Created #2593 to track

[kzc]

@lukastaegert There's a marked improvement in rollup bundling now that it supports package.json sideEffects.

Building https://github.com/mischnic/tree-shaking-example with:

  rollup@1.12.0
  rollup-plugin-babel@4.3.2
  rollup-plugin-commonjs@10.0.0
  rollup-plugin-node-resolve@5.0.0
  rollup-plugin-terser@3.0.0
  rollup-pluginutils@2.7.0
  parcel-bundler@1.10.3
  webpack@4.31.0

produces:

file                size    
------------------  --------
parcel/lodash-es    19.28 KB
rollup/lodash-es    18.22 KB
webpack/lodash-es   20.74 KB

parcel/lodash       91.13 KB
rollup/lodash       69 KB   
webpack/lodash      70.57 KB

parcel/remeda       1.9 KB  
rollup/remeda       1.82 KB 
webpack/remeda      2.74 KB 

parcel/ramda        6.36 KB 
rollup/ramda        6.25 KB 
webpack/ramda       7.16 KB 

parcel/ramdaBabel   6.72 KB 
rollup/ramdaBabel   6.35 KB 
webpack/ramdaBabel  8.39 KB 

parcel/rambda       9.81 KB 
rollup/rambda       589 B   
webpack/rambda      2 KB    

parcel/rambdax      25.57 KB
rollup/rambdax      3.53 KB 
webpack/rambdax     5.09 KB 

@43081j The tree shakability of rxjs is being improved in ReactiveX/rxjs#4769 thanks to @filipesilva.

/cc @mischnic

[Buslowicz]

That is fantastic! Hope the rxjs fix lands there fast and that it will all work correctly now, that would mean much simpler build step for production.

[kzc]

@filipesilva Will the tree shaking fix in PR ReactiveX/rxjs#4769 introduce pure annotations into rxjs/_esm2015 as well?

$ git clone https://github.com/filipesilva/rxjs-6.4.0-without-toplevel-prop-assignment.git

$ grep __PURE__ `find rxjs-6.4.0-without-toplevel-prop-assignment/_esm5 -name '*.js'` | wc -l
     224

$ grep __PURE__ `find rxjs-6.4.0-without-toplevel-prop-assignment/_esm2015 -name '*.js'` | wc -l
       0

Comparison of _esm5 and _esm2015 bundle sizes using rollup@1.12.1 and terser@3.17.0:

$ cat main5.js 
import {Observable} from './rxjs-6.4.0-without-toplevel-prop-assignment/_esm5/index.js';

$ rollup main5.js -f es --silent | terser --toplevel -mc passes=3 | wc -c
      15

$ cat main2015.js 
import {Observable} from './rxjs-6.4.0-without-toplevel-prop-assignment/_esm2015/index.js';

$ rollup main2015.js -f es --silent | terser --toplevel -mc passes=3 | wc -c
   12096

[filipesilva]

Hmmm I imagine that's actually a bug in the build setup for RxJs. I should have a look at that.

[mischnic]

I've updated https://github.com/mischnic/tree-shaking-example to include rxjs (waiting for that rxjs PR...) and react-icons (which parcel has/had issues bundling). I've also added coverage reports for all these cases (though 100% is not always achievable).
Does someone have suggestions for popular packages which (could) really benefit from treeshaking?

[filipesilva]

@mischnic Angular relies a LOT of tree shaking to reduce our bundle size. You can try @angular/core.

If you also want to just check if libraries have side effects without importing anything specific I recommend https://github.com/filipesilva/check-side-effects. This is what I used to detect them in the RxJS and the equivalent Angular PR (angular/angular#29329).

[kzc]

@mischnic Regarding the newly added https://github.com/mischnic/tree-shaking-example result:

rollup/react-icons   29.08 KB  Stmts: 18%, Functions:  6%
webpack/react-icons  10.42 KB  Stmts: 25%, Functions: 15%

Webpack replaces process.env.NODE_ENV with "production" when mode: "production" is used. Rollup does not, so it's getting the development version of the react library.

To compare apples with apples apply this patch:

--- a/rollup.config.js
+++ b/rollup.config.js
@@ -15,7 +15,15 @@ export default [
         exclude: "node_modules/**"
       }),
       commonjs(),
-      terser({ sourcemap: false, toplevel: true })
+      terser({
+        compress: {
+          global_defs: {
+            "process.env.NODE_ENV": "production",
+          },
+        },
+        sourcemap: false,
+        toplevel: true,
+      })
     ],
     output: [{ file: `rollup/${libName}.js`, format: "cjs" }]
   }

to get this result:

rollup/react-icons   9.18 KB   Stmts: 24%, Functions: 10%
webpack/react-icons  10.42 KB  Stmts: 25%, Functions: 15%

Webpack v4 bundles are generally 1.5 KB larger than the rollup equivalent because of its fixed bootstrapping code.

[mischnic]

Thanks, updated.

Webpack v4 bundles are generally 1.5 KB larger than the rollup equivalent because of its fixed bootstrapping code.

This comparison can't perfectly compare apples to apples of course...

[kzc]

@filipesilva afaict rxjs only runs @angular-devkit/build-optimizer on _esm5 to produce the pure annotations, not _esm2015:

https://github.com/ReactiveX/rxjs/blob/01a09789a0a9484c368b7bd6ed37f94d25490a00/.make-packages.js#L56-L76

But since the generated rxjs/package.json specifies _esm5 for "module", I think most ES bundlers will pick that up by default:

module: './_esm5/index.js',
es2015: './_esm2015/index.js'

https://github.com/ReactiveX/rxjs/blob/01a09789a0a9484c368b7bd6ed37f94d25490a00/.make-packages.js#L52-L53

I'm not familiar with the use of the "es2015" tag in package.json. Unless I'm mistaken, _esm2015 will only used if someone overrides the bundler config default, or if rxjs/_esm2015 is explicitly imported in source code.

[filipesilva]

Yes, we use it a lot on Angular packages but it's not a standard. It's part of what we call Angular Package Format (APF, doc here). It's mostly just guidelines on how Angular libs that distribute multiple formats, and different entry points, and should support different consumers should be packaged.

[kzc]

@filipesilva I see. So if Angular is the only consumer of the not-pure-annotated rxjs/_esm2015 presumably when the user builds their app with angular-cli it will run @angular-devkit/build-optimizer on the bundle prior to minification?

[filipesilva]

Right now, yes. We'd like it to be more ergonomic to use the es2015 bundles though...

[shellscape]

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

[Buslowicz]

@shellscape Please reopen this issue as the bug still persists. To replicate the issue, do the following:

1. create a new directory and cd into it
2. npm init
3. npm install @reactivex/rxjs rollup
4. create file index.js with the following content:

import { Observable } from './node_modules/@reactivex/rxjs/dist/esm2015/index.js';
console.log(Observable);

5. rollup index.js --format iife >> index.bundle.js

Expected behavior:
Bundle should only contain Observable with dependencies
Actual behavior:
Bundle contains multiple functions and variables that are not used

[shellscape]

@lukastaegert when you have a moment please evaluate this one given the repro above. I can confirm the behavior, but I don't know enough to know if that's intended, or if that's the cause of something else outside of rollup going sideways.

[Buslowicz]

I don't believe this is intended, as even my WebStorm hints the unused items. Also, other bundlers with tree shaking omit unused code.

[kzc]

Rollup can only tree shake code that is conducive to it. AFAICT the tree shaking fix in the @reactivex/rxjs library was only merged to the 6.x branch a few days ago and has yet to be released.

See: ReactiveX/rxjs#4953 ReactiveX/rxjs#4769.

As a workaround in the meantime you could probably use the rollup flag --no-treeshake.propertyReadSideEffects.

@filipesilva Correct me if I'm wrong.

[lukastaegert]

The unused variables are part of the code because Rollup cannot determine conclusively that the functions that are called to generate them are side-effect free. I do not expect in a short time frame for Rollup to properly understand the mutating way TS helper wrappers work unless we hard code them into Rollup, which would be brittle.

@kzc is right and his suggestion will help. For more dramatic results, you can add --no-treeshake.moduleSideEffects.

But you do not need any of this if you use rollup-plugin-node-resolve which adds support for side-effects hints. This is how all other bundlers do it, but of course this means that you need to write a config file or use the JS API. This also fixes your example.

[Buslowicz]

Interesting, will give it a shot later, thanks for a workaround tip @kzc @lukastaegert.