Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: dynamic modulepreload #7453

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

feat: dynamic modulepreload #7453

wants to merge 31 commits into from

Conversation

wmertens
Copy link
Member

@wmertens wmertens commented Mar 24, 2025

This moves the service worker logic into the qrl handling, adding modulepreload link tags to request preloading chunks that will probably be needed soon.

This allows the browser to do its thing way better, and reduces complexity. (it would also allow some preloading in dev, but only for the SSR result, not implemented)

You can easily test the impact by going to the docs site and clicking the search button.

TODO:

  • test
  • replace qprefetch dispatches with some qwik API call
  • push qwik-city path preloads into bundlegraph
  • deprecate service workers, make them instead uninstall the current SW and use this
  • make this the default, we may not need any strategy any more
  • change jslink strategy to use this
  • fallback to prefetch when modulepreload not supported (iPhone 2017, iPad 2017, iPod Touch. All other browser support it for 1.5 years now. https://iosref.com/ios, https://caniuse.com/link-rel-modulepreload 93%)
  • qwik add service-worker now that the starter no longer includes it

@wmertens wmertens added the COMP: performance This issue is related to a performance problem or bug label Mar 24, 2025
@wmertens wmertens self-assigned this Mar 24, 2025
Copy link

changeset-bot bot commented Mar 24, 2025

🦋 Changeset detected

Latest commit: 392921d

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 Minor
@builder.io/qwik-city Minor
eslint-plugin-qwik Minor
create-qwik Minor

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

@wmertens wmertens force-pushed the dynamic-modulepreload branch 2 times, most recently from b4e3856 to 5c623f5 Compare March 25, 2025 08:33
Copy link

pkg-pr-new bot commented Mar 25, 2025

Open in Stackblitz

npm i https://pkg.pr.new/@builder.io/qwik@7453
npm i https://pkg.pr.new/@builder.io/qwik-city@7453
npm i https://pkg.pr.new/eslint-plugin-qwik@7453
npm i https://pkg.pr.new/create-qwik@7453

commit: 392921d

Copy link
Contributor

github-actions bot commented Mar 25, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
qwik-docs ✅ Ready (View Log) Visit Preview 392921d

@Varixo
Copy link
Member

Varixo commented Mar 25, 2025

Is it a good idea to move fetching bundle graph to the head element?
Currently everything is added at the bottom of page, but if we have some promise to await then interactivity will be broken? For qwikloader we have an option to append it at the top, maybe here also should be such as option?

@wmertens
Copy link
Member Author

@Varixo

The bundlegraph is not that important for initial interactivity, it's better that the browser spends resources on showing the page first.
It is marked with normal priority before the modulepreloads, so depending on bandwidth the browser may or may not fetch it.

All the entry points get higher priority, so they will be available when you click.
The bundlegraph is not needed for QRLs to work, and priority goes to executing the app. Once the bundlegraph is loaded, it injects preloads for everything, but app functioning is not impacted.

const internalDynamicImports = bundle.dynamicImports?.filter((d) => graph[d]) || [];
// If we have a lot of dynamic imports, we don't know which ones are needed, so we don't add any
// This can happen with registry bundles like for routing
if (internalDynamicImports.length > 0 && internalDynamicImports.length < 10) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep the deps.hasSymbols logic as explained in #7417 (comment)

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then we won't preload dynamic imports for library components :-/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm are you sure? I will need to test this. Shouldn't be an issue if libraries use preserveModules, but better to support both ofc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a better solution now! You can adjust the graph in your vite config now, and I already use it to clear the dynamic imports from qwik-city.
I think preloading shiki or threejs or whatever is good if it is actually needed, so in case it's problematic the dev can optimize manually. See the bundle-graph.unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think preloading shiki or threejs or similarly "big" libs can be tricky. We can provide a way to preload those, but it should be optional imo. Otherwise that's also a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think we need to exclude external deps by default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a setting for maximum preload size?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not be needed at all?

@wmertens wmertens force-pushed the dynamic-modulepreload branch from bdea31c to 5d5c54a Compare March 25, 2025 16:57
@wmertens wmertens marked this pull request as ready for review March 25, 2025 16:58
@wmertens wmertens requested review from a team as code owners March 25, 2025 16:58
@wmertens wmertens force-pushed the dynamic-modulepreload branch 4 times, most recently from ee1de36 to 23d530b Compare March 26, 2025 00:04
@wmertens wmertens force-pushed the dynamic-modulepreload branch from 8df1c60 to 74b0661 Compare March 26, 2025 15:45
gioboa
gioboa previously approved these changes Mar 26, 2025
Copy link
Member

@gioboa gioboa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏👏👏@wmertens

@wmertens wmertens force-pushed the dynamic-modulepreload branch 5 times, most recently from 05d51f1 to 115cee1 Compare March 27, 2025 13:31
@wmertens wmertens force-pushed the dynamic-modulepreload branch from f54c44d to a0906bf Compare March 30, 2025 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: performance This issue is related to a performance problem or bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants