Tree-shaking not eliminating some dead code

[mindrones]

• Rollup Version: ^1.1.2
• Operating System (or Browser): Mac OS 10.1.6 (High Sierra)
• Node Version: v11.6.0

How Do We Reproduce?

• git clone https://github.com/mindrones/lambsplittest
• npm i
• npm run build
• check the builds in build/

Expected Behavior

Builds should contain only used functions.

Actual Behavior

Some of the builds contain apparenty unused functions.

For example I don't understand why I'm finding var union = unionBy(identity); in build/mapper.mjs, as union is not referenced anywhere in src/mapper.js or in code used by mapWith and getKey.

build/mapper_manually_tree_shaked.mjs is a working version of the same build but manually tree-shaked, removing the unused parts: here you can see what I removed.

As you can see I also tried to build with the options treeshake: {propertyReadSideEffects: false} and treeshake: {pureExternalModules: true}, with no luck.

[lukastaegert]

unionBy calls pipe which again throws an error if the argument is not an array. As Rollup does not track actually used function arguments for performance reasons at the moment, Rollup must assume that pipe could have the side-effect of throwing an error i.e. Rollup will assume that the line

var union = unionBy(identity);

could throw an error and will therefore retain it. Unfortunately due to the complexity involved I would not expect any improvements any time soon here.

[mindrones]

Ah I see, thanks a lot!

I guess I'm still kinda puzzled because in the original module:

// mapper.js
import * as _ from "lamb";
export const mapWithA = _.mapWith(_.getKey("a"));

mapWith and getKey don't depend on union.

I thought it might have been that Rollup finds union when importing all modules (import * as _ from "lamb";) hence I also tried with:

import {mapWith, getKey} from "lamb";
export const mapWithA = mapWith(getKey("a"));

but with same result, so I'm guessing Rollup is not aware of the fact that union should not be involved? The computed dependency graph might be bugged somehow?

Also, if the reason to keep union is throw, why not including all the functions imported in lamb's src/index.js that make use of a throw, for example setIn?

By the way I tried modifying lambso that all occurrences of throw became return and the builds are the same apart form that substitution, so I'm assuming that Rollup stops tree-shaking when it finds Error types rather than just occurrences of throw?

[kzc]

This line of code is indirectly executed upon loading the module and must be retained due to its side effects:

var union = unionBy(identity);

Once package sideEffects is implemented in Rollup (#2593), it will produce the desired result.

[TrySound]

At least if package author will be definitely sure a package doesn't have side effects. I would refactor the lib to not rely on such "feature".

[kzc]

The package author would be in a position to know what their code does to make that assessment. Refactoring is not always possible or practical. sideEffects is widely used and it would be unfortunate to not take advantage of the hint to reduce code size. The use case is not much different than using pure annotations for logically side effect free code that Rollup wouldn't be able to deduce otherwise.

[lukastaegert]

It will come at some point, time is the limiting factor

[kzc]

@mindrones fwiw if https://github.com/mindrones/lambsplittest is patched to use deep imports it would achieve the same smaller output sought. This would be the net effect of the package sideEffects feature implemented in Rollup with the original simpler code.

diff --git a/src/mapWith.js b/src/mapWith.js
index 8dca92e..a537e5a 100644
--- a/src/mapWith.js
+++ b/src/mapWith.js
@@ -1,3 +1,3 @@
-import * as _ from "./lamb/src/index";
+import { default as mapWith } from "lamb/src/core/mapWith";
 
-export const mapWithA = _.mapWith(x => x.a);
+export const mapWithA = mapWith(x => x.a);
diff --git a/src/mapper.js b/src/mapper.js
index d5113b2..de817ab 100644
--- a/src/mapper.js
+++ b/src/mapper.js
@@ -1,3 +1,4 @@
-import {mapWith, getKey} from "./lamb/src/index";
+import { default as mapWith } from "lamb/src/core/mapWith";
+import { default as getKey } from "lamb/src/object/getKey";
 
 export const mapWithA = mapWith(getKey("a"));
diff --git a/src/splitter.js b/src/splitter.js
index cd43a70..626fdf2 100644
--- a/src/splitter.js
+++ b/src/splitter.js
@@ -1,6 +1,7 @@
-import * as _ from "./lamb/src/index";
+import { default as generic } from "lamb/src/core/generic";
+import { default as partial } from "lamb/src/core/partial";
+import { default as __ } from "lamb/src/core/__";
 
-export const split = _.generic(String.prototype.split);
-
-export const splitBy = x => _.partial(split, [_.__, x]);
+export const split = generic(String.prototype.split);
+export const splitBy = x => partial(split, [__, x]);
 export const splitByDot = splitBy(".");

output	original size	patched size
build/index.mjs	6434	1725
build/lamb.mjs	39016	39016
build/mapWith.mjs	5593	834
build/mapper.mjs	5684	925
build/splitter.mjs	6296	811

[mindrones]

I am still puzzled (sorry! :)
If we are executing lamb's index module (which I wasn't expecting with import {mapWith, getKey} from "lamb"), why aren't we also getting at least all the functions exported in the main lamb's module and throwing directly,

• adapter
• collect
• mapValues
• pickIf
• pipe
• repeat
• setIn
• setPathIn
• skip

in this build?

Thanks!

[mindrones]

@kzc ah that is indeed a great solution, I'll use that while waiting for sideEffects thanks!

[kzc]

The difference is that in order to construct the union export in lamb/src/array/union.js the unionBy function is invoked at import load time and must be retained as result:

var union = unionBy(identity);

export default union;

adapter et al are not invoked at import load time - they are merely declared functions and can safely be dropped if not used.

[mindrones]

I think this can be close after #2844, thanks a lot for that!