-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: support absolute URLs with data-sveltekit-preload-code="viewport"
#12217
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 940f904 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
It would be good to have a test for this. Do we have any tests currently for preloading code? I'm a bit surprised this wouldn't have been caught by our test suite, but maybe this is a hard thing to test? |
I added tests for Note that because the files are named like There are still times when the IntersectionObserver preload can silently fail, such as if the user tries to preload an internal pathname that doesn't exist. If it is a good idea to add a warning in DEV when this happens, I can update the pull request with that warning. |
data-sveltekit-preload-code="viewport"
-_preload_code(/** @type {HTMLAnchorElement} */ (entry.target).href);
+_preload_code(/** @type {HTMLAnchorElement} */ (entry.target).pathname); nvm. It wouldn't run the BTW: |
I moved the href resolver to the However this also means the reroute hook will also run for the |
// viewport | ||
requests.length = 0; | ||
page.locator('#viewport').scrollIntoViewIfNeeded(); | ||
await Promise.all([page.waitForTimeout(100), page.waitForLoadState('networkidle')]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForTimeout
is an anti-pattern. can we avoid it? why do we need both it and networkidle
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForTimeout
is an anti-pattern. can we avoid it? why do we need both it andnetworkidle
?
It was used in the data-sveltekit-preload-data
tests because the hover preload waits before preloading a link. I just copied the test conditions for consistency.
I'll remove the waitForTimeout because they are not needed here
The build test failures seem to be real since they're failing in every browser. Could you take a look at those? |
I'll take a look |
Currently, the IntersectionObserver used for viewport code preloading uses the
.href
attribute of the anchor tag as the pathname to find the route to preload the code for.Internally, the InterSectionObserver gets the
.href
and passes the value to_preload_code
which callsroutes.find
, finding the first route where the route.exec regex match is trueIn Chrome and Firefox, the
.href
value is an absolute URL of the anchor tag value (for examplehttp://localhost:5173/post/123
), and not the relative pathname (/post/123
), resulting in theroute.exec
regex check failing for every route, as it is comparing the relative URLs (from dictionary in app.js) with the absolute URL provided by the browser.This pull request converts the href provided by the browser to a relative URL which then correctly preloads code, as Sveltekit can find the route in the dictionary.
The pull request uses the existing method for converting browser hrefs to relative pathnames as used in
get_navigation_intent
, meaning it will also ignore external URLs and preload code for routes that are rewritten using Sveltekit rewritesPlease don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits