The default shouldPreload logic seems incorrect

[philipwalton]

The default shouldPreload function in this plugin will preload everything that is a dynamic import or has exports (without being a facade chunk). I'm not sure why that criteria was chosen.

Normally you want to preload everything in your initial static module graph, i.e. all the things that the browser would naturally load if given your primary module entry point. The idea is you want to tell the browser ahead of time what's going to be in the graph (so it can make the requests in parallel, without having to discover them).

In some cases you may also want to preload certain modules in the graph of a dynamic entry point (e.g. a router that dynamically loads all routes), but most of the time the reason they're in a dynamic entry point is they're conditionally loaded, so preloading them would defeat the purpose of conditionally loading them.

I think the default logic to determine the list of modules to preload should be:
Every chunk that's in the module graph of all static entry points.

Then, optionally, a developer may choose to specify dynamic entry chunks whose graph they also want to preload.

Lastly, I could see a use case for never preloading certain modules, so it probably makes sense to have a filter.

How does that sound? If you're interested in these changes I'd be happy to submit a PR. To give you an idea of what I'm thinking, here's how I'm currently getting the preload graphs for my entry modules (note: you do have to builds these graphs yourself, as this information isn't exposed by anything in ChunkInfo):

{
  generateBundle(options, bundle) {
    // A mapping of entry chunk names to all dependencies in their
    // static graphs
    const entryChunksMap = {};

    // Loop through all the chunks to detect entries.
    for (const [fileName, chunkInfo] of Object.entries(bundle)) {
      // This logic gets the graphs for both static and dynamic entries
      // but you could imagine making it possible for a user to custom
      // the conditional in this if-statement.
      if (chunkInfo.isEntry || chunkInfo.isDynamicEntry) {
        // A set of chunks in this entries graph.
        const entryGraph = new Set([fileName]);

        // A set of checked chunks to avoid circular dependencies.
        const seen = new Set();

        const imports = chunkInfo.imports.slice();
        let importFileName;
        while (importFileName = imports.shift()) {
          const importChunkInfo = bundle[importFileName];

          entryGraph.add(importChunkInfo.fileName);

          // Add all inner chunk imports to the queue, checking for
          // circular dependencies since chunks can import each other.
          for (const i of importChunkInfo.imports) {
            if (!seen.has(i)) {
              seen.add(i);
              imports.push(i);
            }
          }
        }
        entryChunksMap[chunkInfo.name] = [...entryGraph];
      }
    }

    // Expose this somehow so the developer can create `modulepreload``
    // tags base on these graphs.
    console.log(entryChunksMap);
  }
}

[bennypowers]

Every chunk that's in the module graph of all static entry points.

I think the engine kind of does this anyways. By preloading statically imported modules we gain a slight advantage over the parser but not much else, still it's not a bad idea.

The use case I originally had in mind was lazy-loaded page components. We want them to be downloaded and parsed before the user navigates to them. The problem with that is inferring which modules apply in that case, so I cast a wide net instead. It's a naive implementation, which is why I left shouldPreload configurable, as an escape hatch.

What do you think about this:

1. deprecate shouldPreload option
2. introduce preload option, of type { 'static' | 'dynamic' | ChunkInfo => Boolean }

Where the keywords 'static' and 'dynamic' refer to your default and mine, respectively. There would be no default, the plugin would throw if the user didn't choose.

[philipwalton]

I think the engine kind of does this anyways.

Chrome (the only browser to implement modulepreload as of today) does not do this. I've discussed with the engineers implementing modulepreload and they said they couldn't optimize sub-module preloading (O(n^2) processing time) to be faster than what they could do with the entire graph enumerated (O(n) processing), so they opted to not implement it.

As a result, we continue to recommend including modulepreload tags for all modules in the graph.

The use case I originally had in mind was lazy-loaded page components. We want them to be downloaded and parsed before the user navigates to them.

I get the use case, but wouldn't you get the same affect by statically importing them rather than dynamically importing them? And if code splitting is what you want, you could get that via the manualChunks config option, which (as long as it didn't self-initialize) would also allow you to load it with low importance via something like this at the foot of your document:

<script type=module importance=low href="...">

Regardless, though, I do think there's value in being able to use modulepreload with things being dynamically imported, so there should definitely be a way to do it.

What do you think about this:

1. deprecate shouldPreload option
2. introduce preload option, of type { 'static' | 'dynamic' | ChunkInfo => Boolean }

The hard part in all this (why I think there should be a plugin rather than manually writing your own generateBundle() hook) is getting the list of dependencies in the graph for each entry point.

If the static option returned all entries in the static graph, I'd be in favor of something like that (same for the dynamic option), and for the ChunkInfo => Boolean option, if returning true meant it included those chunks + all the the dependencies in their graph, I also think that would be worthwhile.

However if ChunkInfo => Boolean just either includes that chunk or doesn't based on return value, I don't think you're getting much more than you'd get by writing your own generateBundle() hook.

What do you think? If you'd prefer to have this plugin not manage generating dependency graphs for entry points, I'd be happy to write a separate plugin that does do that (rollup-plugin-modulepreload-graphs or whatever).

[bennypowers]

Let me try to summarize your suggestion, and please let me know if I've covered your concerns:

{
  // preloads all modules that are included in the graph,
  // accd to your code above
  preload: 'static',
  // everything 'static' does', but includes `import()`s in those modules,
  // and all their static dependencies
  preload: 'dynamic', 
  // when true, preload the module. 
  preload: (x: ChunkInfo) => Boolean 
}

That sounds really useful to me

also, based on your very clear and informative explanation of Chrome team's recommendations above, I'm comfortable making static the default, since I think it has so much utility as a 'drop in' plugin for all users, not just my original use case.

[philipwalton]

Actually, before continuing, I want to confirm some behavior about Rollup. At the moment, it looks like Rollup always lists all imports in the graph for entry chunks, and it looks like this is actually the intended behavior.

If this is the case, then adding logic to traverse the graph is not need -- that being said, I'm not sure this is a good feature on Rollup's part, but I need to confirm. Let me get back to you!