Skip to content

fix: ensure beforeNavigate works on other navigations after clicking a download link #9747

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

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
6bcfec8
add test
eltigerchino Apr 22, 2023
d6d8a1f
fix test
eltigerchino Apr 22, 2023
be5b1dc
only call before_navigate if external link is unloading page
eltigerchino Apr 22, 2023
42c683a
rename external_link flag
eltigerchino Apr 22, 2023
0fb55dc
changeset
eltigerchino Apr 22, 2023
e1a861e
format
eltigerchino Apr 23, 2023
0c892ed
Merge branch 'master' into fix-implicit-download-before-navigate
eltigerchino May 18, 2023
91cfd72
revert changes and update tests
eltigerchino May 18, 2023
4beaab5
comment
eltigerchino May 18, 2023
32e68ca
oopsie accidentally removed this
eltigerchino May 18, 2023
98d2caa
forgot to revert this too
eltigerchino May 18, 2023
33ad7f6
fix test
eltigerchino May 19, 2023
ed3d004
add fix
eltigerchino May 19, 2023
0eedd67
maybe this will fix the ubuntu tests?
eltigerchino May 19, 2023
4f16369
Revert "maybe this will fix the ubuntu tests?"
eltigerchino May 19, 2023
d69fcda
Merge branch 'master' into fix-implicit-download-before-navigate
eltigerchino Aug 16, 2023
dc283bb
Merge branch 'master' into fix-implicit-download-before-navigate
eltigerchino Dec 2, 2023
dc7f1b0
Merge branch 'main' into fix-implicit-download-before-navigate
eltigerchino May 18, 2024
8e46929
fix merge differences
eltigerchino May 18, 2024
ea529ba
try this?
eltigerchino May 19, 2024
b97cba1
use locator
eltigerchino May 19, 2024
82c04c7
try this nonsense
eltigerchino May 20, 2024
267840a
fix test names
eltigerchino May 21, 2024
8d01c5d
remove unused link
eltigerchino May 21, 2024
6d8cf4c
fix flaky tests?
eltigerchino May 21, 2024
76af069
fix native navigations that download a file
eltigerchino May 21, 2024
8ff1e86
wait for download to finish
eltigerchino May 21, 2024
4ce3f24
apparently other tests wait for networkidle so this should be fine
eltigerchino May 21, 2024
645ceb1
prettier
eltigerchino May 21, 2024
0b48eab
remove throwing error if downloads
eltigerchino May 21, 2024
f89f269
cancel download because it interferes with navigation
eltigerchino May 21, 2024
c83dfab
need to wait for event first probably?
eltigerchino May 21, 2024
a5c0efd
Merge branch 'main' into fix-implicit-download-before-navigate
benmccann Jul 9, 2024
a9c1801
Merge branch 'main' into fix-implicit-download-before-navigate
eltigerchino Jul 24, 2024
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/great-schools-cross.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: ensure `beforeNavigate` works on other navigations after clicking a download link
18 changes: 17 additions & 1 deletion packages/kit/src/runtime/client/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,16 @@ function clear_onward_history(current_history_index, current_navigation_index) {
* Returns a `Promise` that never resolves (to prevent any
* subsequent work, e.g. history manipulation, from happening)
* @param {URL} url
* @returns {Promise<any>}
*/
function native_navigation(url) {
location.href = url.href;
return new Promise(() => {});
return new Promise((resolve) => {
// if we're still on the same page, it was probably a download
Copy link
Member

Choose a reason for hiding this comment

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

this has me a bit uneasy. Are there any situations where this can be true but it is not a download?

Copy link
Member Author

@eltigerchino eltigerchino Jul 8, 2024

Choose a reason for hiding this comment

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

only if native_navigation is being used to navigate to the same page. However, it's currently only used as a fallback when navigating to another page. e.g., clicking a link that has a different href than the current page

Copy link
Member

Choose a reason for hiding this comment

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

hmm, what if for some reason the response redirected you back to where you came from?
Are we unable to detect that something is a download before starting a navigation that really isn't one?

Copy link
Member Author

@eltigerchino eltigerchino Oct 10, 2024

Choose a reason for hiding this comment

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

It's tricky. Some downloads are only detectable once the response headers have been downloaded and Content-Disposition: attachment is present. Another option is to revert this and let the developer be explicit about which links are downloads with the download attribute so they don't run into this issue.

if (location.href !== url.href) {
resolve(undefined);
}
});
}

function noop() {}
Expand Down Expand Up @@ -1293,6 +1299,13 @@ async function navigate({
}),
404
);

// do nothing if we downloaded a file and are still on the same page
if (!navigation_result) {
stores.navigating.set(null);
nav.fulfil(undefined);
return;
}
}

// if this is an internal navigation intent, use the normalized
Expand Down Expand Up @@ -1998,6 +2011,9 @@ function _start_router() {

before_navigate_callbacks.forEach((fn) => fn(navigation));
}
// We need to reset the flag manually here because clicking on an
// implicit download link does not reset it for the next navigation.
navigating = false;

if (should_block) {
e.preventDefault();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export function GET() {
return new Response('ok', {
headers: {
'Content-Disposition': 'attachment'
}
});
}
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
<script>
import { beforeNavigate } from '$app/navigation';

const implicit_download_url = '/navigation-lifecycle/before-navigate/download';

let times_triggered = 0;
let unload = false;
let navigation_type;

beforeNavigate(({ cancel, type, willUnload, to }) => {
times_triggered++;
unload = willUnload;
navigation_type = type;

// we don't cancel the `beforeunload` event for the implicit download link
// because it's needed for the download to occur.
if (to?.url.pathname === implicit_download_url) return;

if (!to?.route.id?.includes('redirect')) {
cancel();
}
Expand All @@ -20,6 +28,7 @@
<a href="/navigation-lifecycle/before-navigate/prevent-navigation?x=1">self</a>
<a href="https://google.com" target="_blank" rel="noreferrer">_blank</a>
<a href="https://google.de">external</a>
<!-- svelte-ignore a11y-invalid-attribute -->
<a download href="">external</a>
<a download href="/">explicit download</a>
<a href={implicit_download_url}>implicit download</a>

<pre>{times_triggered} {unload} {navigation_type}</pre>
16 changes: 15 additions & 1 deletion packages/kit/test/apps/basics/test/cross-platform/client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ test.describe('Navigation lifecycle functions', () => {
expect(await page.innerHTML('pre')).toBe('2 false goto');
});

test('beforeNavigate is triggered after clicking a download link', async ({ page }) => {
test('beforeNavigate is triggered after clicking an explicit download link', async ({ page }) => {
await page.goto('/navigation-lifecycle/before-navigate/prevent-navigation');

const download = page.waitForEvent('download');
Expand All @@ -234,6 +234,20 @@ test.describe('Navigation lifecycle functions', () => {
await expect(page.locator('pre')).toHaveText('1 false link');
});

test('beforeNavigate is triggered after clicking an implicit download link', async ({ page }) => {
await page.goto('/navigation-lifecycle/before-navigate/prevent-navigation');

const download = page.waitForEvent('download');
await page.locator('a[href="/navigation-lifecycle/before-navigate/download"]').click();
await (await download).cancel();

await expect(page.locator('pre')).toHaveText('1 true link');

await page.locator('a[href="/navigation-lifecycle/before-navigate/a"]').click();

await expect(page.locator('pre')).toHaveText('2 false link');
});

test('afterNavigate calls callback', async ({ page, clicknav }) => {
await page.goto('/navigation-lifecycle/after-navigate/a');
expect(await page.textContent('h1')).toBe(
Expand Down
Loading