[🐞] over-preloading due to inaccurate bundle-graph static import graph

[maiieul]

Which component is affected?

Qwik Runtime

Describe the bug

Running a client build for a qwik app in 1.16 (or 2.0.0-beta-7) results in an incorrect q-manifest which results in an inaccurate bundle-graph. This is because Rollup's generateChunks merges static node_modules into manualChunks bundles aliases.

Reproduction

https://github.com/maiieul/qwik-overpreloading-inaccurate-bundle-graph-static-imports-graph

Steps to reproduce

Run build.client look at the "alert.tsx_Description_component_MExiH8uFvn4.js" component in the q-manifest. It includes the orgins for a lot of node_modules (e.g tailwind-merge, css-tree, etc.). Those are imported by @qwik-ui/utils which the Alert component is importing. The problem is that when rollup sees this, it merges the @qwik-ui/utils node_modules into the "alert.tsx_Description_component_MExiH8uFvn4.js" bundle, even though the node_modules in question are also used in other bundles, resulting in those other bundles importing "alert.tsx_Description_component_MExiH8uFvn4.js" and its static+dynamic dependencies.

Tip: you can run the debugger build.client from within vscode to see this in action.

System Info

🙈

Potential solutions

We can solve this in two ways:

1) fix Rollup

The problem comes from generateChunks -> getChunkAssignments -> getChunkDefinitionsFromManualChunks -> addStaticDependenciesToManualChunk -> if (!(dependency instanceof ExternalModule || modulesInManualChunks.has(dependency))) modulesToHandle.add(dependency).

We can fix it by removing the addStaticDependenciesToManualChunk logic.

2) Improve our manualChunks logic

The current manualChunks logic will take related QRL segments outputs from the optimizer, and merge them together:

    const { pathId } = parseId(id);
    const segment = segments.get(normalizePath(pathId));
    if (segment) {
      const { hash } = segment;
      const chunkName = (opts.entryStrategy as SmartEntryStrategy).manual?.[hash] || segment.entry;
      if (chunkName) {
        return chunkName;
      }
    }

This not only works for the app's QRL segments, but also for node_modules QRL segments (e.g. import { Accordion } from "@qwik-ui/headless"; will contain many segments.

This means that for the remaining non QRL ids and node_modules ids, we can safely return them into separate bundles and group node_modules by name, taking into account their size.

Solution 2 gives us more control, but we might end up bundling together bundles that were explicitly set to be separate by a library, or create multiple bundles for code that will always be used together.

Edit: updated solution 1.

[wmertens]

Well you should definitely report this at rollup (or rolldown).
Solution 2 seems ok to use in the meantime - we already have code that keeps track of bundle size somewhere in the git history.

[maiieul]

Well you should definitely report this at rollup (or rolldown).

Yes ofc I am going to.

[maiieul]

Hmmm it seems option 2 can break the bundling for certain libraries. For example I get the following error when click on an SPA Link:

it seems to be because "property-information-lib" imports booleanish, and booleanish is defined in a "property-information-util" as const booleanish = increment(). Apparently this can happen with circular dependencies.

Components on the other hand work perfectly.

Either way this approach seems to be a bit brittle compared to the Rollup fix.

Edit: Funny (harmless) side effect, this aproach also leads to quite a few empty bundles (10 to a 100 depending on project size).

Edit 2: I can confirm that this can lead to the "Cannot Access XXX before initialization" error on many libraries. This happens when there is a circular dependency between two files.

Edit 3 – here's the code I used:

  function getNodeModulesRelativePath(id: string): string | null {
    // Expect a normalized posix-like path
    const nm = '/node_modules/';
    const idx = id.lastIndexOf(nm);
    if (idx === -1) {
      return null;
    }
    // Portion after the last node_modules/
    const sub = id.slice(idx + nm.length);
    if (!sub) {
      return null;
    }
    // Drop query/hash if any
    const clean = sub.split('?')[0].split('#')[0];
    const parts = clean.split('/');
    if (parts.length < 2) {
      // Not enough info to determine package and file
      return null;
    }
    // All directory parts between package root and file name
    const dirParts = parts.slice(0, parts.length - 1);
    if (dirParts.length === 0) {
      return null;
    }
    return 'vendor-' + dirParts.join('-');
  }
  function manualChunks(
    id: string,
    ctx: Parameters<Extract<Rollup.OutputOptions['manualChunks'], Function>>[1]
  ) {
    const module = ctx?.getModuleInfo(id);
    console.log('manualChunks', id, module);
    if (opts.target === 'client') {
      if (
        // The preloader has to stay in a separate chunk if it's a client build
        // the vite preload helper must be included to prevent breaking circular dependencies
        id.endsWith('@builder.io/qwik/build') ||
        /[/\\]qwik[/\\]dist[/\\]preloader\.[cm]js$/.test(id) ||
        id === '\0vite/preload-helper.js'
      ) {
        return 'qwik-preloader';
      } else if (/qwik[\\/]dist[\\/]qwikloader\.js$/.test(id)) {
        return 'qwik-loader';
      }
      if (/\/(qwik|core)\/dist\/core.*js$/.test(id)) {
        return 'core';
      }
      if (/\/(qwik-city|router)\/lib\/index.qwik.*js$/.test(id)) {
        return 'qwik-city';
      }
    }

    const relatedVendor = getNodeModulesRelativePath(normalizePath(pathId));
    if (relatedVendor) {
      console.log('relatedVendor', id, relatedVendor);
      return relatedVendor;
    }

    return null;
  }

[maiieul]

Another issue with solution 2, is that for example certain libs are better bundled by rollup. It makes sense that rollup has optimized logic to group modules together based on their imports graph.

For example with lodash-es, all the helpers are at node_modules/lodash-es/[helper].js. In this case it is very hard for us to create separate bundles for each helper, whereas rollup does this automatically.

It seems the bundling is always more optimized with rollup (when it comes to non QRL node_modules).