Inlining of deps from empty chunks with preserveModules doesn't check the moduleSideEffects flag

[dmichon-msft]

• Rollup Version: 1.29.1
• Operating System (or Browser): Windows 10
• Node Version: 10.18.0

Search terms:
treeshaking
moduleSideEffects
preserveModules
empty imports

Related:
#3057
#3109

How Do We Reproduce?

Using the module layout here:
REPL
Specify preserveModules: true and treeShaking.moduleSideEffects: false

I understand the contract of the moduleSideEffects flag to mean, for a given module:

• true: All modules imported later than this module may have an implicit dependency on this module, and therefore this module must not be moved to be later in the load order (but may potentially be moved earlier)
• false: No modules have an implicit dependency on this module, and thus it may be safely moved later in the load order

Expected Behavior

// main.js
import { a } from '../a.js';
import { d } from '../other.js';

console.log(a + d);

Actual Behavior

// main.js
import { a } from '../a.js';
import '../b.js';
import { d } from '../other.js';

console.log(a + d);

Addressing this may be as simple as introducing a check in inlineChunkDependencies:

                if (!dep.isEmpty) {
+                   // In preserveModules mode, only inline the dep if it has side effects (being used elsewhere is not a side effect)
+                   if (!this.graph.preserveModules || !dep.facadeModule || dep.facadeModule.moduleSideEffects) {
                        this.dependencies.push(dep);
+                   }
                }

Edit: added info about moduleSideEffects expectations
Edit: added link to 3109

[lukastaegert]

The actual solution I implemented was a little more complicated as pruning dependencies should ideally happen on module level, not on chunk level. Nevertheless, there is a pending fix now: #3369 !

[lukastaegert]

Solved in rollup@2.0.0