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

Improve prefetch behaviour for browsers #10999

Merged
merged 4 commits into from
May 15, 2024
Merged

Improve prefetch behaviour for browsers #10999

merged 4 commits into from
May 15, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented May 10, 2024

Changes

Closes #10464

  • This PR makes all prefetch strategies to use <link rel="prefetch"> if supported, or else fall back to fetch().
  • It also applies the same for the programmatic prefetch() API, unless the with: 'link' | 'fetch' option is passed. The with option is deprecated as it's better to use link whenever possible.

More explanation for why this change is made at #10464 (comment)

Testing

The existing tests should pass. Testing is a bit tricky because this mainly matters for Firefox and Safari but we don't have those setup. I think manual testing should be enough for now as the prefetch code is isolated and doesn't changed often.

Docs

withastro/docs#8246

Copy link

changeset-bot bot commented May 10, 2024

🦋 Changeset detected

Latest commit: 75b93d9

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

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label May 10, 2024
@ematipico
Copy link
Member

ematipico commented May 10, 2024

@bluwy what should we do to test it on Firefox? I can help

@bluwy
Copy link
Member Author

bluwy commented May 10, 2024

We probably need to update playwright.config.js projects to also test on Firefox etc. But we don't want to run all tests on Firefox, so we may need a second playwright config that only runs specific tests? And I think that should be enough, we can also run it for the view transitions tests.

We also need to update this to install the firefox etc binaries:

astro/package.json

Lines 33 to 34 in 1c59059

"test:e2e": "cd packages/astro && pnpm playwright install chromium && pnpm run test:e2e",
"test:e2e:match": "cd packages/astro && pnpm playwright install chromium && pnpm run test:e2e:match",

@ematipico
Copy link
Member

Awesome, I can make a PR for that if you're happy

}
// Prefetch with link if supported
else if (
document.createElement('link').relList?.supports?.('prefetch') &&
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it somewhat expensive to do this on every link? Could it be done once?

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 benchmarked this locally and there isn't a big difference in perf from what I can tell compared to caching, so I kept this direct approach, and to save some bytes.

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

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

The conditional checks look right to me. Glad we're getting with the times!

@ematipico
Copy link
Member

I added Firefox to the e2e tests, however some of the tests fail e.g. view transitions

@bluwy bluwy merged commit 5f353e3 into main May 15, 2024
13 checks passed
@bluwy bluwy deleted the better-prefetch branch May 15, 2024 13:11
@astrobot-houston astrobot-houston mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro's prefetch doesn't work in non-Chromium-based browsers
4 participants