Skip to content

fix(prefetch-sw): only prefetch direct imports #6853

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

Merged
merged 6 commits into from
Sep 10, 2024
Merged

fix(prefetch-sw): only prefetch direct imports #6853

merged 6 commits into from
Sep 10, 2024

Conversation

wmertens
Copy link
Member

@wmertens wmertens commented Sep 2, 2024

  • use prefetch sw with docs

It was adding dynamic imports, which is not correct. Those should be handled by Link prefetching and Insights.

Furthermore, the graph is reduced by not including dependencies that other dependencies already import.

@wmertens wmertens requested review from a team as code owners September 2, 2024 12:42
@wmertens wmertens enabled auto-merge (squash) September 2, 2024 12:42
Copy link

changeset-bot bot commented Sep 2, 2024

🦋 Changeset detected

Latest commit: da895f6

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

Copy link

pkg-pr-new bot commented Sep 2, 2024

Open in Stackblitz

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

commit: da895f6

Copy link
Contributor

github-actions bot commented Sep 2, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

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

Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

I don't think removing dynamic import is the right fix here. It is true that we are currently over fetching do to dynamic imports, but I think better fix is to encode which imports are dynamic into the graph (for example negative numbers are dynamic imports)

SW can than make decisions on this.

Also it would be great if we could differentiate dynamic import form useTask (which will very likely be needed) from a route navigation, which is much less likely to be needed.

This is why I think the fix here is to encode more information so that SW can make more intelligent decisions.

In the past we ran into issues where SW would fetch static imports but not dynamic and than the site was slow because the initial click handler would get processed but subsequent useTask etc... would be slow.

@wmertens
Copy link
Member Author

wmertens commented Sep 4, 2024

@mhevery We can differentiate imports, handlers, segments, and dynamic imports. We can differentiate by negative numbers indicating the type of what follows, -1 handlers, -2 segments, -3 dynamic.

Alternatively, Qwik loader can just tell us which handlers are actually on the page and those then get fetched opportunistically, no differentiation needed, only direct imports. What do you think?

🤔 Or indeed any dynamically imported segments that are not handlers are likely to be tasks etc.

Ah and we can sort by module size, should we fetch big or small modules first?

Not for this PR, but the prefetch SW is also missing:

  • limit concurrent downloads
  • clear speculative fetches on navigation
  • a way for qwik city to declare a path-to-imports mapping for link prefetching. I'm thinking qwik-city extends the manifest with paths and their imports.
  • support for insights

@wmertens wmertens force-pushed the fix-prefetch branch 2 times, most recently from 0e1931a to 99e45f9 Compare September 6, 2024 13:59
@wmertens
Copy link
Member Author

wmertens commented Sep 6, 2024

@mhevery I now detect tasks (by the fact that they also export _hW) and fetch them with lower priority. Other dynamic imports are left up to the qprefetch events.

WDYT?

shairez
shairez previously approved these changes Sep 10, 2024
Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

My understanding is that you and Shai had a chat and decided to add this as a temporary work around, and than fix it later. In that case this is good to go.

@wmertens wmertens merged commit b4cd308 into main Sep 10, 2024
18 checks passed
@wmertens wmertens deleted the fix-prefetch branch September 10, 2024 23:20
@github-actions github-actions bot mentioned this pull request Sep 10, 2024
wmertens added a commit that referenced this pull request Sep 24, 2024
* fix(prefetch-sw): only prefetch direct imports

- dynamic imports should be fetched separately
- don't list deps that are already deps-of-deps

* feat(prefetch-sw): reduce fetch prio for dynamic tasks

* perf(prefetch-sw): pre-process bundle graph

* perf(prefetch-sw): don't prefetch indirect deps of indirect deps

* perf(prefetch-sw): limit concurrent low prio fetches

* fix(manifest): better import path handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants