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

Add fetchpriority option for link + Fix prefetch url in dev mode #6824

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

Conversation

GrandSchtroumpf
Copy link
Contributor

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos
  • Infra

Description

prefetch strategy linkInsert always has fetchpriority set to "auto" which might result into busying the browser at load time. With linkFetchPriority we can set the bundle prefetch to "low" to give an indication to the browser.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have ran pnpm change and documented my changes
  • I have made corresponding changes to the Qwik docs
  • Added new tests to cover the fix / functionality -> no existing test infra for this par for the code

@GrandSchtroumpf GrandSchtroumpf requested review from a team as code owners August 26, 2024 15:20
Copy link

changeset-bot bot commented Aug 26, 2024

🦋 Changeset detected

Latest commit: 383abb0

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

@GrandSchtroumpf GrandSchtroumpf requested a review from a team as a code owner August 26, 2024 15:32
Copy link

pkg-pr-new bot commented Aug 26, 2024

Open in Stackblitz

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

commit: 2d39b31

Copy link
Contributor

github-actions bot commented Aug 26, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

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

@thejackshelton
Copy link
Member

LGTM! Except for the DOMContentLoaded listener. Which if I recall we were using just to test?

Verified that this does fix the paths in dev with modulepreload, dev mode is super snappy 😄

@GrandSchtroumpf
Copy link
Contributor Author

@thejackshelton I removed the addEventListener and verify that modulepreload href works great for dev & production now.
Using html-append or js-append with modulepreload makes first interaction in devmode much faster.

@thejackshelton thejackshelton changed the title Add fetchpriority option for link Add fetchpriority option for link + Fix prefetch url in dev mode Aug 29, 2024
thejackshelton
thejackshelton previously approved these changes Aug 29, 2024
Copy link
Member

@thejackshelton thejackshelton left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

LGTM besides the api update

packages/docs/src/routes/api/qwik/index.md Outdated Show resolved Hide resolved
@thejackshelton thejackshelton dismissed wmertens’s stale review September 1, 2024 21:02

He ran the API update changes

thejackshelton
thejackshelton previously approved these changes Sep 1, 2024
Copy link
Contributor

@shairez shairez left a comment

Choose a reason for hiding this comment

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

thanks @GrandSchtroumpf ! 🙏

@shairez
Copy link
Contributor

shairez commented Sep 18, 2024

For some reason, the tests are failing after I merged main

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.

6 participants