fix: under-prefetching — bundle graph missing dynamic dependencies

[maiieul]

What is it?

• Bug

Description

Repro: https://github.com/maiieul/qwik-under-prefetching-repro -> TLDR: open.value && <Comp /> has always led to under-prefetching. This could be worked around (as was the case on the qwik.dev docs) by using qwikVite's entryStrategy.manual (https://qwik.dev/docs/guides/bundle/).

I managed to get a pretty good result by checking for each bundle if their dynamic dependencies are entries, reducing the under-prefetching a lot. Then I noticed that removing manualChunks logic on top of that completely gets rid of it with the qwik-city ServiceWorkerRegister. The qwik PrefetchServiceWorker also works but is a bit buggy.



Results https://github.com/QwikDev/qwik/pull/7417:


• On qwik UI, with the qwik PrefetchServiceWorker there is a bit of over-prefetching (but that’s due to specific PrefetchServiceWorker bugs): ~6s TTI. With the qwik-city ServiceWorkerRegister it is ~2s TTI so ~3x faster. Without the change, it is ~1.5s, but with under-prefetching.

• There is zero under-prefetching.
• Since I removed manualChunks, there are less chunks outputted overall, which means the interactions feel snappier. For example the qwik.dev docs search now takes ~50ms to serve all the chunks from SW instead of ~300ms.





Remaining issues/questions:

• The qwik PrefetchServiceWorker doesn't prioritize the chunks on user interaction. This means if all the chunks need 5s to be prefetched, the app won’t be interactive before that point, and also that the user must re-interact to actually get a result.

• Both service workers can't prioritize chunks need for user interaction before DOMContentLoaded. For the qwik-city service worker for example, it can start to prefetch chunks before DOMContentLoaded, but cannot re-prioritize. This would be a good optimization to have for 3G networks.
• There is now a bit more of over-prefetching due to the static imports being wrong. For example copy-icon component on qwik UI statically imports the toggle-group.js, which dynamically imports all of the toogle-group chunks. So instead of over-prefetching only the toggle-group.js, we now over-prefetch all the chunks that come with it :/. I’m hopeful we can find a way to fix this. Probably a setting in rollup.

• It is unclear to me whether clearTransitiveDeps and the direct <dynamic> check do anything useful. They don’t seem to change anything on qwik.dev and qwik-ui.com I’m concerned these might lead to under-prefetching in some untested situations.

Bonus: this also fixes the docs search 🥳

Malus: I disabled qwik insights because it was logging lots of error on the network tab 🤔

Fixes #4149

list of PRs that created problems with prefetching:

• perf: preload css fonts #4673 -> change visible tasks to tasks change the symbols hashes, which broke the entryStrategy.manual for the docs search.
• fix: fix up PrefetchServiceWorker with multiple Qwik containers #6468 -> removal of document.dispatchEvent(new CustomEvent("qprefetch",{detail:${JSON.stringify(data)}})) broke the qwik-city SW. Brought back in fix: qwikcity sw SSR prefetching #7286.
• fix(optimizer): let the bundler handle entries #6670 -> introduced severe over-prefetching by letting rollup handle entries. Also introduced returning segments (hooks) in manualChunks, therefore preventing rollup to bundle those chunks with others, which affects the bundle-graph, creating even more over-prefetching but also under-prefetching.
  fix(prefetch-sw): only prefetch direct imports #6853 -> Fixed over-prefetching by only allowing tasks dynamic dependencies to be prefetched, because prefetching all kinds of dynamic imports can for example prefetch all dynamic imports from external dependencies (e.g. all of the shiki syntax definitions 😬 ), but also introduced more under-prefetching because some non tasks chunks also need to be prefetched.

Checklist

• My code follows the developer guidelines of this project
• I performed a self-review of my own code
• I added a changeset with pnpm change
• I made corresponding changes to the Qwik docs
• I added new tests to cover the fix / functionality

[changeset-bot]

🦋 Changeset detected

Latest commit: 81b3c3b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@builder.io/qwik	Patch
eslint-plugin-qwik	Patch
@builder.io/qwik-city	Patch
create-qwik	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in Stackblitz

npm i https://pkg.pr.new/@builder.io/qwik@7417

npm i https://pkg.pr.new/@builder.io/qwik-city@7417

npm i https://pkg.pr.new/eslint-plugin-qwik@7417

npm i https://pkg.pr.new/create-qwik@7417

commit: 81b3c3b

[github-actions]

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name	Status	Preview	Last Commit
qwik-docs	✅ Ready (View Log)	Visit Preview	81b3c3b

[wmertens]

@maiieul I think you can add the isPlan and we can merge this already as a bit of an improvement?

[maiieul]

Yup, I think it's actually quite a big improvement. Without isEntry we were missing a lot of direct level 0 dynamic imports. This also happens to fix the docs search slowness btw 🥹

[wmertens]

needs a little cleanup but I think we can merge this even though #7453 will follow soon

[maiieul]

Ok, in #7453 I just removed all of it and only disallow more than 10 dynamic deps because we're probably not going to guess right. That works fine too it seems.

We need the hasSymbols guard, otherwise we will prefetch external dynamic deps again (e.g. shiki). 10 sounds like a reasonable safeguard, but I wonder if it's needed at all. The dynamic deps graph seems to stop on its own after a certain number of nodes. There is still a bit of over-prefetching on qwik-ui but I'm quite sure it's because some static deps that have dynamic deps aren't being tree-shaken somehow.