Avoid unnecessary facade dependency inlining

[guybedford]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

This removes the unnecessary dependency inlining that is applying for cases of facade modules referencing other facade modules, thanks to @lukastaegert :)

[lukastaegert]

Some initial comments:
So far as I can see, there are exactly two issues solved:

1. If a namespace object is included for an entry point that is imported somewhere else, a facade is created. This skips having an empty import to a chunk that just contains a namespace object.
2. If an entry point has more than one user-defined facade but the entry point itself just contains reexports, the empty import is removed.

I think those issues may be solved more elegantly and with less effort, but I am not sure yet. As for why I think this is about it and why there are probably not other improvements: Chunk dependencies are already very much pruned a-priori based on actual side-effects and configured moduleSideEffects. The rather elaborate logic for this is located here: https://github.com/rollup/rollup/blob/master/src/Module.ts#L341-L388

Most notably, I do not expect ever for an unused external import to remain in a chunk if it has no side-effects. And if there are just imports without a binding from a module, a chunk will not get a dependency on that module, which in the end means that a chunk will never depend on a side-effect-free chunk unless a binding is imported. The only exception here are facade chunks, but for me it makes much more sense then to move this logic here https://github.com/rollup/rollup/blob/master/src/Chunk.ts#L119-L133
but maybe I am wrong. So the logic could be: If a module only exports stuff from other chunks than the facaded chunk and no module in the facaded chunk has a side-effect, then instead of adding this chunk as a dependency to the facade chunk, we should copy the dependency list of the facaded chunk to the facade chunk.

Also there was no reason to undeprecate pureExternalModules as this is completely superseded by the much more powerful moduleSideEffects. The moved tests are useful, but instead they should be copied and be modified to use moduleSideEffects instead. For some reason, I mostly created function tests for this feature, some more form tests would help definitely Also, some of your test results are wrong, at least for the array test, other MUST be included as an empty import.

[guybedford]

I see that the basis use case of chunk pruning is actually solved looking at eg:

http://rollupjs.org/repl/?version=2.9.1&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QiUyMHV0aWwlMjAlN0QlMjBmcm9tJTIwJy4lMkZ1dGlsLmpzJyUzQiU1Q251dGlsKCklM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJ1dGlsLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMColMjBmcm9tJTIwJy4lMkZ1dGlsMi5qcyclM0IlNUNuZXhwb3J0JTIwZnVuY3Rpb24lMjB1dGlsMiUyMCgpJTIwJTdCJTVDbiU1Q3Rjb25zb2xlLmxvZygndXRpbDInKSUzQiU1Q24lN0QlNUNuJTJGJTJGJTIwY29uc29sZS5sb2coJ3NpZGUlMjBlZmZlY3QnKSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0FmYWxzZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJ1dGlsMi5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjBmdW5jdGlvbiUyMHV0aWwlMjAoKSUyMCU3QiU1Q24lNUN0Y29uc29sZS5sb2coJ3V0aWwnKSUzQiU1Q24lN0QlMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIybWFpbjIuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyaW1wb3J0JTIwJTdCJTIwdXRpbDIlMjAlN0QlMjBmcm9tJTIwJy4lMkZ1dGlsLmpzJyUzQiU1Q251dGlsMigpJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIybWFpbjMuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyaW1wb3J0JTIwJTdCJTIwdXRpbCUyMCU3RCUyMGZyb20lMjAnLiUyRnV0aWwuanMnJTNCJTVDbnV0aWwoJ2FnYWluJyklM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

In this case, let me ask your suggestion how you would solve the original test case then?

[guybedford]

One benefit to this PR approach though might be that by handling this sort of "wiring up" only at the end of the optimization process, it may enable more optimizations to happen in the middle that eg change the wiring.

[guybedford]

Also, I think this PR might solve some preserveModules bugs where bindings wouldn't have otherwise been available due to the lack of inlineChunkModules expanding dependencies to include them as I did see some cases where not running that full expansion made the render phase fail.

[rollup-bot]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#chunk-pruning

or load it into the REPL:
https://rollupjs.org/repl/?circleci=11041

[codecov]

Codecov Report

Merging #3552 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3552   +/-   ##
=======================================
  Coverage   96.29%   96.29%           
=======================================
  Files         176      176           
  Lines        6043     6048    +5     
  Branches     1781     1783    +2     
=======================================
+ Hits         5819     5824    +5     
  Misses        112      112           
  Partials      112      112           

Impacted Files	Coverage Δ
src/Chunk.ts	99.79% <100.00%> (+<0.01%)	⬆️
src/Module.ts	98.90% <100.00%> (+<0.01%)	⬆️
src/ModuleLoader.ts	99.53% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cbcc8d...1c97de9. Read the comment docs.

[lukastaegert]

I pushed an alternative solution to your branch that solves all your issues in mostly six lines of code + some refactoring by fixing what was actually broken, namely createFacadeChunk. So what was broken is that a facade chunk just had a single dependency, the chunk of the original module, but no others.

• That means without hoistTransitiveDependencies, reexports from other chunks were missing (as you noticed)
• And also of course that there may be no reason to have a dependency on that chunk

So what I did is I reused the logic that determines the actually necessary dependencies after tree-shaking when side-effects are taken into account (or rather the result of the first run as it is cached).

The only dependencies we need to have are then the relevant dependencies of the original module. If one of them ends up being in the chunk of the original module, then of course we have a dependency on that chunk. This would include also all side-effects only dependencies of the original module. The only dependency that is not covered if the original module itself has a side-effect, which is why in this case, we always add a dependency on the original chunk.

One benefit to this PR approach though might be that by handling this sort of "wiring up" only at the end of the optimization process, it may enable more optimizations to happen in the middle that eg change the wiring.

I do not think there is any more optimization potential on module level here, at least to my understanding the current algorithm is pretty much maxed out since #3354 except for the fact that too many facades were created. But I think this is solved now as well.

The next quantum leap would be statement level chunking, and I have some vague ideas building on the current algorithm, but so far this is still mostly future talk as the next big things I want to focus on is moving the chunking to the generate phase and fixing the hashing for good.

[lukastaegert]

Also, I think this PR might solve some preserveModules bugs where bindings wouldn't have otherwise been available due to the lack of inlineChunkModules expanding dependencies to include them as I did see some cases where not running that full expansion made the render phase fail.

If you have a failing test case, please post it. I would be rather surprised by this as facades are not used for preserve modules, but we had some default export issues here which should be solved in recent Rollup.

[guybedford]

Thanks @lukastaegert for offering the suggested approach here, this looks great and is a huge help to have a solution to this problem.

Yes I do see that side effects fully affect the chunking algorithm, which I hadn't realised you had implemented already. I must admit I don't see where in #3354 any side effect or binding checks are being done though, but did follow that refactoring and the entry nesting concept is great. I wonder if it is possible to define entry nesting hierarchies at the entry configuration level? Sounds like it would be similar to the manual chunks work, but with less "manual" work!

If you have a failing test case, please post it.

The thing I noticed was that if you disable dependency inlining for all output, you get test failures that are not inlining test failures. So basically if you comment out the inlineChunkDependencies call, some unrelated code error paths get hit. I'm pretty sure it wasn't due to other code changes I had made too. Tested this and all is good!

[guybedford]

GitHub won't let me approve my own PR, but this has my 👍

[guybedford]

Wondering if you have any ideas how we could make the aliases be the later entry points in order, rather than the first entry points in order as works here?

It's not a big thing, but would be nice to just lock this behaviour down either way.

[lukastaegert]

I added a small change so that both the order in the bundle object and the order of facade creation respects the order configured by the user. I.e. the first chunk for a given entry becomes the actual chunk and later ones become facades, and the order between those is preserved.

Previously, later entries would "overtake" earlier entries because loading the later ones would just return the module instance of the earlier modules, making them "load faster" and receive their name earlier.

[guybedford]

I just tried out disabling the inlining and all works well actually - seems it must have been due to an unrelated code path.

[lukastaegert]

I must admit I don't see where in #3354 any side effect or binding checks are being done though

Which is because I messed up here, it is actually that much more aptly named #3369. I sounds that it is just about avoiding imports, but it actually builds an optimized graph based on tree-shaking and side-effect information. I think this example sums it up really well.

[guybedford]

Ah I see, I hadn't realised you'd done all that already! Yes that was exactly what this PR was attempting with binding and side effect checks.

Thanks for working these through @lukastaegert.