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.

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

fix: only call afterNavigate once on start when SSR is disabled #13593

Merged
merged 3 commits into from
Mar 23, 2025
+61 −32
Merged

fix: only call afterNavigate once on start when SSR is disabled #13593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4ff154
add fix and test
eltigerchino Mar 17, 2025
67994fc
changeset
eltigerchino Mar 17, 2025
1a183bf
format
eltigerchino Mar 17, 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/angry-ravens-eat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: only call `afterNavigate` once on app start when SSR is disabled
4 changes: 2 additions & 2 deletions packages/kit/src/exports/public.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,7 @@ export interface NavigationTarget {
}

/**
* - `enter`: The app has hydrated
* - `enter`: The app has hydrated/started
* - `form`: The user submitted a `<form>` with a GET method
* - `leave`: The user is leaving the app by closing the tab or using the back/forward buttons to go to a different document
* - `link`: Navigation was triggered by a link click
Expand Down Expand Up @@ -1101,7 +1101,7 @@ export interface OnNavigate extends Navigation {
export interface AfterNavigate extends Omit<Navigation, 'type'> {
/**
* The type of navigation:
* - `enter`: The app has hydrated
* - `enter`: The app has hydrated/started
* - `form`: The user submitted a `<form>`
* - `link`: Navigation was triggered by a link click
* - `goto`: Navigation was triggered by a `goto(...)` call or a redirect
Expand Down
48 changes: 28 additions & 20 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 @@ -320,8 +320,10 @@ export async function start(_app, _target, hydrate) {
if (hydrate) {
await _hydrate(target, hydrate);
} else {
await goto(app.hash ? decode_hash(new URL(location.href)) : location.href, {
replaceState: true
await navigate({
type: 'enter',
url: resolve_url(app.hash ? decode_hash(new URL(location.href)) : location.href),
replace_state: true
});
}

Expand Down Expand Up @@ -479,20 +481,22 @@ function initialize(result, target, hydrate) {

restore_snapshot(current_navigation_index);

/** @type {import('@sveltejs/kit').AfterNavigate} */
const navigation = {
from: null,
to: {
params: current.params,
route: { id: current.route?.id ?? null },
url: new URL(location.href)
},
willUnload: false,
type: 'enter',
complete: Promise.resolve()
};
if (hydrate) {
/** @type {import('@sveltejs/kit').AfterNavigate} */
const navigation = {
from: null,
to: {
params: current.params,
route: { id: current.route?.id ?? null },
url: new URL(location.href)
},
willUnload: false,
type: 'enter',
complete: Promise.resolve()
};

after_navigate_callbacks.forEach((fn) => fn(navigation));
after_navigate_callbacks.forEach((fn) => fn(navigation));
}

started = true;
}
Expand Down Expand Up @@ -1373,7 +1377,7 @@ function _before_navigate({ url, type, intent, delta }) {

/**
* @param {{
* type: import('@sveltejs/kit').Navigation["type"];
* type: import('@sveltejs/kit').NavigationType;
* url: URL;
* popped?: {
* state: Record<string, any>;
Expand Down Expand Up @@ -1407,7 +1411,10 @@ async function navigate({
token = nav_token;

const intent = await get_navigation_intent(url, false);
const nav = _before_navigate({ url, type, delta: popped?.delta, intent });
const nav =
type === 'enter'
? create_navigation(current, intent, url, type)
: _before_navigate({ url, type, delta: popped?.delta, intent });

if (!nav) {
block();
Expand All @@ -1423,7 +1430,7 @@ async function navigate({

is_navigating = true;

if (started) {
if (started && nav.navigation.type !== 'enter') {
Copy link
Member

Choose a reason for hiding this comment

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

is this necessary? When could started be true but the type enter?

Copy link
Member Author

@eltigerchino eltigerchino Mar 21, 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 was only added for type narrowing. TypeScript doesn’t know that we won’t use navigate with the “enter” navigation type after the app has started. Meanwhile, navigating.current has been typed to never have the “enter” navigation type.

stores.navigating.set((navigating.current = nav.navigation));
}

Expand Down Expand Up @@ -2847,10 +2854,11 @@ function reset_focus() {
}

/**
* @template {import('@sveltejs/kit').NavigationType} T
* @param {import('./types.js').NavigationState} current
* @param {import('./types.js').NavigationIntent | undefined} intent
* @param {URL | null} url
* @param {Exclude<import('@sveltejs/kit').NavigationType, 'enter'>} type
* @param {T} type
*/
function create_navigation(current, intent, url, type) {
/** @type {(value: any) => void} */
Expand All @@ -2867,7 +2875,7 @@ function create_navigation(current, intent, url, type) {
// Handle any errors off-chain so that it doesn't show up as an unhandled rejection
complete.catch(() => {});

/** @type {import('@sveltejs/kit').Navigation} */
/** @type {Omit<import('@sveltejs/kit').Navigation, 'type'> & { type: T }} */
const navigation = {
from: {
params: current.params,
Expand Down
13 changes: 13 additions & 0 deletions packages/kit/test/apps/basics/src/routes/no-ssr/after-navigate/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<script>
import { afterNavigate } from '$app/navigation';
let count = 0;
let type = '';
afterNavigate((event) => {
count += 1;
type = event.type;
});
</script>

<p>{type.toString()} {count}</p>
17 changes: 10 additions & 7 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 @@ -335,8 +335,8 @@
}
});

test.describe('Page options', () => {
test('applies generated component styles with ssr=false (hides announcer)', async ({
test.describe('SPA mode / no SSR', () => {
test('applies generated component styles (hides announcer)', async ({
page,
clicknav,
get_computed_style
Expand All @@ -346,9 +346,7 @@

expect(await get_computed_style('#svelte-announcer', 'position')).toBe('absolute');
});
});

test.describe('SPA mode / no SSR', () => {
test('Can use browser-only global on client-only page through ssr config in handle', async ({
page,
read_errors
Expand All @@ -358,7 +356,7 @@
expect(read_errors('/no-ssr/browser-only-global')).toBe(undefined);
});

test('Can use browser-only global on client-only page through ssr config in +layout.js', async ({
test('can use browser-only global on client-only page through ssr config in +layout.js', async ({
page,
read_errors
}) => {
Expand All @@ -367,7 +365,7 @@
expect(read_errors('/no-ssr/ssr-page-config')).toBe(undefined);
});

test('Can use browser-only global on client-only page through ssr config in +page.js', async ({
test('can use browser-only global on client-only page through ssr config in +page.js', async ({
page,
read_errors
}) => {
Expand All @@ -376,14 +374,19 @@
expect(read_errors('/no-ssr/ssr-page-config/layout/inherit')).toBe(undefined);
});

test('Cannot use browser-only global on page because of ssr config in +page.js', async ({
test('cannot use browser-only global on page because of ssr config in +page.js', async ({
page
}) => {
await page.goto('/no-ssr/ssr-page-config/layout/overwrite');
await expect(page.locator('p')).toHaveText(
'This is your custom error page saying: "document is not defined (500 Internal Error)"'
);
});

test('afterNavigate is only called once during start', async ({ page }) => {
await page.goto('/no-ssr/after-navigate');
await expect(page.locator('p')).toHaveText('enter 1');
});
});

// TODO SvelteKit 3: remove these tests
Expand Down Expand Up @@ -965,7 +968,7 @@
expect(page).toHaveURL(offline_url);
});

test('data-sveltekit-preload-data error does not block user navigation', async ({

Check warning on line 971 in packages/kit/test/apps/basics/test/client.test.js

View workflow job for this annotation

GitHub Actions / test-kit (18, ubuntu-latest, chromium) 

retries: 2
page,
context,
browserName
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/test/apps/basics/test/cross-platform/client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
test.describe.configure({ mode: 'parallel' });

test.describe('a11y', () => {
test('resets focus', async ({ page, clicknav, browserName }) => {

Check warning on line 12 in packages/kit/test/apps/basics/test/cross-platform/client.test.js

View workflow job for this annotation

GitHub Actions / test-kit-cross-browser (18, windows-2019, chromium, dev) 

retries: 2
const tab = browserName === 'webkit' ? 'Alt+Tab' : 'Tab';

await page.goto('/accessibility/a');
Expand All @@ -33,7 +33,7 @@
expect(await page.evaluate(() => document.documentElement.getAttribute('tabindex'))).toBe(null);
});

test('applies autofocus after a navigation', async ({ page, clicknav }) => {

Check warning on line 36 in packages/kit/test/apps/basics/test/cross-platform/client.test.js

View workflow job for this annotation

GitHub Actions / test-kit-cross-browser (18, windows-2019, chromium, dev) 

retries: 2
await page.goto('/accessibility/autofocus/a');

await clicknav('[href="/accessibility/autofocus/b"]');
Expand Down Expand Up @@ -257,7 +257,7 @@
);
});

test('afterNavigate properly removed', async ({ page, clicknav }) => {
test('onNavigate returned function is only called once', async ({ page, clicknav }) => {
await page.goto('/navigation-lifecycle/after-navigate-properly-removed/b');
await clicknav('[href="/navigation-lifecycle/after-navigate-properly-removed/a"]');
await clicknav('[href="/navigation-lifecycle/after-navigate-properly-removed/b"]');
Expand Down Expand Up @@ -867,7 +867,7 @@
await expect(page.locator('h1')).toHaveText('target: 1');
});

test('responds to <form method="GET"> submission without reload', async ({ page }) => {

Check warning on line 870 in packages/kit/test/apps/basics/test/cross-platform/client.test.js

View workflow job for this annotation

GitHub Actions / test-kit-server-side-route-resolution (build) 

retries: 2
await page.goto('/routing/form-get');

expect(await page.textContent('h1')).toBe('...');
Expand All @@ -888,7 +888,7 @@
expect(await page.textContent('h3')).toBe('bar');
});

test('responds to <form target="_blank"> submission with new tab', async ({ page }) => {

Check warning on line 891 in packages/kit/test/apps/basics/test/cross-platform/client.test.js

View workflow job for this annotation

GitHub Actions / test-kit-cross-browser (18, macOS-latest, webkit, build) 

retries: 2
await page.goto('/routing/form-target-blank');

let tabs = page.context().pages();
Expand All @@ -902,7 +902,7 @@
expect(tabs.length > 1);
});

test('responds to <button formtarget="_blank" submission with new tab', async ({ page }) => {

Check warning on line 905 in packages/kit/test/apps/basics/test/cross-platform/client.test.js

View workflow job for this annotation

GitHub Actions / test-kit-cross-browser (18, ubuntu-latest, firefox, build) 

retries: 2
await page.goto('/routing/form-target-blank');

let tabs = page.context().pages();
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,7 +1007,7 @@ declare module '@sveltejs/kit' {
}

/**
* - `enter`: The app has hydrated
* - `enter`: The app has hydrated/started
* - `form`: The user submitted a `<form>` with a GET method
* - `leave`: The user is leaving the app by closing the tab or using the back/forward buttons to go to a different document
* - `link`: Navigation was triggered by a link click
Expand Down Expand Up @@ -1083,7 +1083,7 @@ declare module '@sveltejs/kit' {
export interface AfterNavigate extends Omit<Navigation, 'type'> {
/**
* The type of navigation:
* - `enter`: The app has hydrated
* - `enter`: The app has hydrated/started
* - `form`: The user submitted a `<form>`
* - `link`: Navigation was triggered by a link click
* - `goto`: Navigation was triggered by a `goto(...)` call or a redirect
Expand Down