Skip to content

fix: preload data on tap after code was preloaded on hover #13530

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.

Sign up for GitHub

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 4 commits into from
Mar 4, 2025
Merged

fix: preload data on tap after code was preloaded on hover #13530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ee6418
add failing test
eltigerchino Mar 3, 2025
94f8b7c
check if it requests data twice on tap
eltigerchino Mar 3, 2025
ae4894c
preload data on tap if we only preloaded code on hover
eltigerchino Mar 3, 2025
0633140
changeset
eltigerchino Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silver-pigs-report.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: correctly preload data on `mousedown`/`touchstart` if code was preloaded on hover
64 changes: 37 additions & 27 deletions packages/kit/src/runtime/client/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1636,25 +1636,29 @@ if (import.meta.hot) {
});
}

/** @typedef {(typeof PRELOAD_PRIORITIES)['hover'] | (typeof PRELOAD_PRIORITIES)['tap']} PreloadDataPriority */

function setup_preload() {
/** @type {NodeJS.Timeout} */
let mousemove_timeout;
/** @type {Element} */
let current_a;
/** @type {PreloadDataPriority} */
let current_priority;

container.addEventListener('mousemove', (event) => {
const target = /** @type {Element} */ (event.target);

clearTimeout(mousemove_timeout);
mousemove_timeout = setTimeout(() => {
void preload(target, 2);
void preload(target, PRELOAD_PRIORITIES.hover);
}, 20);
});

/** @param {Event} event */
function tap(event) {
if (event.defaultPrevented) return;
void preload(/** @type {Element} */ (event.composedPath()[0]), 1);
void preload(/** @type {Element} */ (event.composedPath()[0]), PRELOAD_PRIORITIES.tap);
}

container.addEventListener('mousedown', tap);
Expand All @@ -1674,11 +1678,14 @@ function setup_preload() {

/**
* @param {Element} element
* @param {number} priority
* @param {PreloadDataPriority} priority
*/
async function preload(element, priority) {
const a = find_anchor(element, container);
if (!a || a === current_a) return;

// we don't want to preload data again if the user has already hovered/tapped
const interacted = a === current_a && priority >= current_priority;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure? I could've hovered like 10 minutes ago if I left for another tab, or to eat a sandwich or something, then came back and hovered again -- not sure we'd want to serve stale data in that case

Copy link
Member Author

@eltigerchino eltigerchino Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this has already been the behaviour since we cache the load data on hover. Additionally, @dummdidumm added the a === current_a skip condition as part of the server-side route resolution PR and it does help to skip more of the preload code while hovering over the same link (previously, you could move your mouse across the same link and this function would get called every 20ms).

Maybe we should add a timer to expire the load data cache?

Copy link
Member

Choose a reason for hiding this comment

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

We can investigate that separately - for now this looks good to fix the bug

if (!a || interacted) return;

const { url, external, download } = get_link_info(a, base, app.hash);
if (external || download) return;
Expand All @@ -1687,31 +1694,34 @@ function setup_preload() {

// we don't want to preload data for a page we're already on
const same_url = url && get_page_key(current.url) === get_page_key(url);

if (!options.reload && !same_url) {
if (priority <= options.preload_data) {
current_a = a;
const intent = await get_navigation_intent(url, false);
if (intent) {
if (DEV) {
void _preload_data(intent).then((result) => {
if (result.type === 'loaded' && result.state.error) {
console.warn(
`Preloading data for ${intent.url.pathname} failed with the following error: ${result.state.error.message}\n` +
'If this error is transient, you can ignore it. Otherwise, consider disabling preloading for this route. ' +
'This route was preloaded due to a data-sveltekit-preload-data attribute. ' +
'See https://svelte.dev/docs/kit/link-options for more info'
);
}
});
} else {
void _preload_data(intent);
if (options.reload || same_url) return;

if (priority <= options.preload_data) {
current_a = a;
// we don't want to preload data again on tap if we've already preloaded it on hover
current_priority = PRELOAD_PRIORITIES.tap;

const intent = await get_navigation_intent(url, false);
if (!intent) return;

if (DEV) {
void _preload_data(intent).then((result) => {
if (result.type === 'loaded' && result.state.error) {
console.warn(
`Preloading data for ${intent.url.pathname} failed with the following error: ${result.state.error.message}\n` +
'If this error is transient, you can ignore it. Otherwise, consider disabling preloading for this route. ' +
'This route was preloaded due to a data-sveltekit-preload-data attribute. ' +
'See https://svelte.dev/docs/kit/link-options for more info'
);
}
}
} else if (priority <= options.preload_code) {
current_a = a;
void _preload_code(/** @type {URL} */ (url));
});
} else {
void _preload_data(intent);
}
} else if (priority <= options.preload_code) {
current_a = a;
current_priority = priority;
void _preload_code(/** @type {URL} */ (url));
}
}

Expand Down
7 changes: 7 additions & 0 deletions packages/kit/test/apps/basics/src/routes/data-sveltekit/preload-data/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,10 @@
</div>

<a id="tap" href="/data-sveltekit/preload-data/target" data-sveltekit-preload-data="tap">tap</a>

<a
id="hover-then-tap"
href="/data-sveltekit/preload-data/target"
data-sveltekit-preload-code="hover"
data-sveltekit-preload-data="tap">hover for code then tap for data</a
>
5 changes: 5 additions & 0 deletions packages/kit/test/apps/basics/src/routes/data-sveltekit/preload-data/target/+page.server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export function load() {
return {
a: 1
};
}
60 changes: 54 additions & 6 deletions packages/kit/test/apps/basics/test/client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,37 +887,44 @@ test.describe('data-sveltekit attributes', () => {
req
.response()
.then(
(res) => res.text(),
(res) => res?.text(),
() => ''
)
.then((response) => {
if (response.includes('this string should only appear in this preloaded file')) {
.then((text) => {
if (text?.includes('this string should only appear in this preloaded file')) {
requests.push(req.url());
}
});
}

if (req.url().includes('__data.json')) {
requests.push(req.url());
}
});

await page.goto('/data-sveltekit/preload-data');
await page.locator('#one').hover();
await page.locator('#one').dispatchEvent('touchstart');
await Promise.all([
page.waitForTimeout(100), // wait for preloading to start
page.waitForLoadState('networkidle') // wait for preloading to finish
]);
expect(requests.length).toBe(1);
expect(requests.length).toBe(2);

requests.length = 0;
await page.goto('/data-sveltekit/preload-data');
await page.locator('#two').hover();
await page.locator('#two').dispatchEvent('touchstart');
await Promise.all([
page.waitForTimeout(100), // wait for preloading to start
page.waitForLoadState('networkidle') // wait for preloading to finish
]);
expect(requests.length).toBe(1);
expect(requests.length).toBe(2);

requests.length = 0;
await page.goto('/data-sveltekit/preload-data');
await page.locator('#three').hover();
await page.locator('#three').dispatchEvent('touchstart');
await Promise.all([
page.waitForTimeout(100), // wait for preloading to start
page.waitForLoadState('networkidle') // wait for preloading to finish
Expand All @@ -932,7 +939,7 @@ test.describe('data-sveltekit attributes', () => {
page.waitForTimeout(100), // wait for preloading to start
page.waitForLoadState('networkidle') // wait for preloading to finish
]);
expect(requests.length).toBe(1);
expect(requests.length).toBe(2);
});

test('data-sveltekit-preload-data network failure does not trigger navigation', async ({
Expand Down Expand Up @@ -1001,6 +1008,47 @@ test.describe('data-sveltekit attributes', () => {
expect(page).toHaveURL('/data-sveltekit/preload-data/offline/slow-navigation');
});

test('data-sveltekit-preload-data tap works after data-sveltekit-preload-code hover', async ({
page
}) => {
/** @type {string[]} */
const requests = [];
page.on('request', (req) => {
if (req.resourceType() === 'script') {
req
.response()
.then(
(res) => res?.text(),
() => ''
)
.then((text) => {
if (text?.includes('this string should only appear in this preloaded file')) {
requests.push(req.url());
}
});
}

if (req.url().includes('__data.json')) {
requests.push(req.url());
}
});

await page.goto('/data-sveltekit/preload-data');
await page.locator('#hover-then-tap').hover();
await Promise.all([
page.waitForTimeout(100), // wait for preloading to start
page.waitForLoadState('networkidle') // wait for preloading to finish
]);
expect(requests.length).toBe(1);

await page.locator('#hover-then-tap').dispatchEvent('touchstart');
await Promise.all([
page.waitForTimeout(100), // wait for preloading to start
page.waitForLoadState('networkidle') // wait for preloading to finish
]);
expect(requests.length).toBe(2);
});

test('data-sveltekit-reload', async ({ baseURL, page, clicknav }) => {
/** @type {string[]} */
const requests = [];
Expand Down
Loading