Option to turn off the "avoiding the waterfall" feature

[philipwalton]

Feature Use Case

I recently started noticing a lot of import statements in my built modules that don't actually import any symbols. Here's an example:

import './core-js-4d767783.mjs';
import { p as polyfillDynamicImport } from './polyfillDynamicImport-171c7873.mjs';
import './object-assign-d3be3a6c.mjs';
import './react-1f5f6245.mjs';
import './date-fns-466f94c0.mjs';
import './highlight.js-2ff46463.mjs';
import './style-inject-d8a06a3d.mjs';
import './CodeBlock-a5cc5a9f.mjs';
import './timeOrigin-692e90ef.mjs';
import './App-dc9eaee3.mjs';
import { m as main } from './main-eb2319cc.mjs';

When I started looking into why, I realized this is actually intended to be a feature: "avoiding the waterfall"

At first I thought this was really cool, but then, based on some stuff I learned from some Chrome engineers who implemented modules, I realized this probably isn't a good practice. What developers should really be using to avoid the waterfall is modulepreload (which works for both static and dynamic imports, via Link headers).

The problem with using "real" imports to preload a bunch of modules is, when fetching them, the spec says to run the #internal-module-script-graph-fetching-procedure algorithm, and when using modulepreload, the spec says to run the #fetch-a-single-module-script algorithm. The former algorithm will also fetch sub-dependencies, while the later algorithm will only fetch the single module.

The problem with running the #internal-module-script-graph-fetching-procedure algorithm on a bunch of modules that likely have common sub-dependencies is you'll end up processing many of those sub-dependencies more than once, so the processing time ends up being O(n^2) rather than O(n) for modulepreload.

Feature Proposal

While the current approach is probably better than a completely unoptimized load (without modulepreload), it's worse than an optimized load (because of the O(n^2) issue mentioned and the fact that the file sizes are larger than they need to be).

As a result, I propose Rollup add an option to turn off this feature (it's something modulepreload plugins could turn off automatically for users of their plugins).

As long as ChunkInfo.imports contains the complete (deep) graph of all imports needed for a particular entry chunk, it should be easy enough for developers to generate their own modulepreload lists. However, if removing the imports-as-modulepreload statements from the final output also means they're removed from the ChunkInfo.imports object, I'd also suggest adding another property (perhaps importsDeep) to the ChunkInfo object, so it continues to be easy to generate a modulepreload list from the bundle object.

What do you think?

[lukastaegert]

I see your point, but I'm not yet sure how useful this actually is, considering there appears to be a single browser that supports modulepreload. But of course, developer interest is the most important measure here.

As for the deep chunk dependency info, I do not see why adding additional information is needed as this can be aggregated easily by traversing the chunks (and I would also know "how deep" the dependency actually is if I am interested). Considering this is something that would be done in a dedicated plugin anyway, I would prefer to avoid the additional interface if possible.

you'll end up processing many of those sub-dependencies more than once, so the processing time ends up being O(n^2) rather than O(n) for modulepreload

If that happens, that sounds like a bad implementation of the spec to me (or a problem with the spec, but I hope not). Such a scenario will already happen if a file has many dependencies that again all depend on a third file, e.g. a shared library. I really hope this library will not be processed again once for each import but there is some flag saying "a, we are already working on that".

[lukastaegert]

Another thing I keep wondering about, totally unrelated to your request but related to what you wrote: Does it really make sense to preload dynamic dependencies? The danger I see is that when the browser is busy pre-loading dynamic dependencies, it may be stealing bandwidth from dependencies I actually need right away to load dependencies I may not need at all and certainly not immediately. Or is the intended course to add the script tags for dynamic dependencies dynamically at some point?

[philipwalton]

If that happens, that sounds like a bad implementation of the spec to me (or a problem with the spec, but I hope not).

This was my thought as well when I first learned about the O(n^2) issue, but I was assured by the Chrome engineers who implemented modules that using modulepreload was more efficient than listing each module script in the graph.

I really hope this library will not be processed again once for each import but there is some flag saying "a, we are already working on that".

I don't think the entire module is processed again, I think it's just that the #internal-module-script-graph-fetching-procedure algorithm needs to be run each time.

Re: having a flag indicating that algorithm is being run, I believe the overhead of adding such a flag (queuing a task after completion, etc.) would be similar to the overhead of running the algorithm itself (but that's just my guess, I'd have to ask about that).

I'll admit, I'm not sure as to the degree of extra processing this introduces; it could be that the performance impact is only noticeable if the graph is huge. I'm happy to dig further into it though if you'd like.

---

Another thing I keep wondering about, totally unrelated to your request but related to what you wrote: Does it really make sense to preload dynamic dependencies?

As a general rule, I don't think it does.

The problem is lots of developers use dynamic import as a means of code splitting, e.g. splitting out route-specific code. So in cases where your server is responding with HTML for a specific route, it might make sense to preload those route-specific chunks.

For cases where the dynamic import is conditional based on user-interaction, I don't think it makes sense to preload the resource. Or at minimum, you'd:

• defer preloading until after the load event
• preload using service worker
• preload using importance="low" (not supported yet for modulepreload)

[philipwalton]

Update: I followed up with some of the Chrome engineers to get to the bottom of the O(n^2) issue with import statements, and it turns out what I was initially told was not fully correct.

To summarize the main takeaways of our conversation:

• The time complexity of using an import statement per chunk is O(n)
• The time complexity of using <script type=module> per chunk is O(n^2) (at least in the current Chrome implementation)
• Using modulepreload is still technically faster than using import statements, but for a small graph (e.g. <100 chunks) the difference is probably negligible

I do still think an option to turn the feature off would be nice (it makes the files smaller if you're already implementing modulepreload), but given that only Chrome implements modulepreload at the moment, I wouldn't say this is a high-priority issue.

[bathos]

Unsafe ‘waterfall avoidance’ hoisting has led to three bugs for us in six months. Each time it was that modules (without side effects of their own) were being hoisted above modules with important side effects (polyfills, etc).

We also get no benefit from this. The modules in question have always already been sent with http2 server push or are available in the service worker cache. It would be great if we could disable this.

(If anyone has immediate advice, something they know I can do to force this import hoisting to respect side effect ordering, I’d be forever grateful.)

[dmichon-msft]

I just spent the last couple of days trying to figure out why my rollup builds with preserveModules: true and moduleSideEffects: false were injecting side-effect-only imports in the resulting js files. Today I finally found the right search terms to locate this issue.

My scenario is that I am attempting to use rollup to shake an ESM module graph and convert it to a different format (AMD) before handing it off to an existing bundler, in order to facilitate migrating to a pure-ESM development flow. Unfortunately, due to the behavior of this "feature", my resulting modules contain much more code than they did originally (due to all the extraneous imports), which makes the approach a non-starter due to inflating the resulting bundles. The modules will end up being bundled into the same file anyway, so the extra imports just result in requirejs doing significant unnecessary work.

I would very much appreciate being able to turn this function of rollup off via config, or if there is a hook I can use to remove imports of side-effect free modules categorically via plugin?

[lukastaegert]

Fix at #3353. There should be some comments form my side, though:

@bathos

Unsafe ‘waterfall avoidance’ hoisting has led to three bugs for us in six months. Each time it was that modules (without side effects of their own) were being hoisted above modules with important side effects (polyfills, etc).

I am nearly 100% sure that this has nothing to do with import hoisting. Import hoisting does not affect the execution order at all. The problem is instead that Rollup does not guarantee exact execution order when either external dependencies or code-splitting involved. I am quite sure, though, that #3354 will improve the situation for you.

I added an FAQ section to explain how to add polyfills and why there could be issues: https://github.com/rollup/rollup/pull/3353/files#diff-4c58bec2df8beef1009c4ed8f417566eR66-R98

The solution (add it to each static entry first) will work once #3354 is released. Also have a look at the example in the description of #3354 to understand the issue better.

@dmichon-msft

I just spent the last couple of days trying to figure out why my rollup builds with preserveModules: true and moduleSideEffects: false were injecting side-effect-only imports in the resulting js files. Today I finally found the right search terms to locate this issue.

I am sorry to tell you that preserveModules automatically deactivates this feature, so this is NOT what has hit you. Instead I am quite sure it is this: #3109 (comment) I think at some point we will add that moduleSideEffects also works for modules that are part of the graph. At the moment, it only has an effect for modules from which no import is ever used.

[bathos]

@lukastaegert Thanks! It’s definitely possible (and, given what you said, apparently very likely) that I was too quick to identify the incorrect ordering behavior as hoisting. IIRC when I wrote that it was after a few hours of what is happening why r u doing this to me please stop why, so when I found what looked like an explanation in the docs, I assumed that had to be it, but I didn’t ever confirm it.

There are no external deps in play in our case, but we do have a bunch of dynamic imports — which in turn import modules that are imported by other chunks. It looked very much like the polyfill example in #3354.

Thanks immensely for identifying and fixing the actual issue.