Skip to content

fix: change page.url when calling pushState(...) with a new url #13221

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

Closed
wants to merge 1 commit into from
Closed

fix: change page.url when calling pushState(...) with a new url #13221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 deletions packages/kit/src/runtime/client/client.js
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

looks like the same changes as for pushState should be applied to export function replaceState(url, state). They seem to be very much the same and probably should share a common function with an arg like method with values either being pushState or replaceState. The error messages would be the same with method replacements and then there is only other thing when calling history and should be taken care of with history[method](opts, '', url).

But I don't know if the team still wants to keep them separate.

Original file line number Diff line number Diff line change
Expand Up @@ -1958,17 +1958,22 @@ export function pushState(url, state) {
}

update_scroll_positions(current_history_index);
url = resolve_url(url);
Copy link

Choose a reason for hiding this comment

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

to make sure that a new instance of URL is created as resolve_url will simply return the same instance if url was already a URL vs a string. And if the instance is the same replacing url on the page will not trigger a change.

Suggested change
url = resolve_url(url);
url = new URL(resolve_url(url));

const change = page.url.href !== url.href ? 1 : 0;
Copy link

Choose a reason for hiding this comment

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

it's a boolean so not needed this here.

Suggested change
const change = page.url.href !== url.href ? 1 : 0;


const opts = {
[HISTORY_INDEX]: (current_history_index += 1),
[NAVIGATION_INDEX]: current_navigation_index,
[PAGE_URL_KEY]: page.url.href,
[NAVIGATION_INDEX]: (current_navigation_index += change),
Copy link

Choose a reason for hiding this comment

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

I don't think this should be updated since no navigation actually takes place, only the url changes.

Suggested change
[NAVIGATION_INDEX]: (current_navigation_index += change),
[NAVIGATION_INDEX]: current_navigation_index,

[PAGE_URL_KEY]: url.href,
[STATES_KEY]: state
};

history.pushState(opts, '', resolve_url(url));
history.pushState(opts, '', url);
has_navigated = true;

if (change) {
Copy link

Choose a reason for hiding this comment

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

since it's a boolean

Suggested change
if (change) {
if (page.url.href !== url.href) {

page.url = url;
Copy link

Choose a reason for hiding this comment

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

Not 100% sure but maybe update_url(url) would be the call here to make sure that stores are also updated.

Suggested change
page.url = url;
update_url(url);

}
page.state = state;
root.$set({ page });

Expand Down
32 changes: 32 additions & 0 deletions packages/kit/test/apps/basics/src/routes/state/url/[slug]/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<script>
import { pushState } from '$app/navigation';
import { page } from '$app/state';
import { untrack } from 'svelte';

let count = $state(0);

$effect(() => {
page.url;
untrack(() => {
count += 1;
});
});
</script>

<button
type="button"
class="before"
onclick={() => {
pushState('', { s: 'test' });
}}>slug = before</button
>
<button
type="button"
class="after"
onclick={() => {
pushState('/state/url/after', { s: 'test' });
Copy link

Choose a reason for hiding this comment

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

also need a test to pushState with new URL(...) to check if it's reactive with a URL instance.

}}>slug = after</button
>

<h1>{page.url.pathname}</h1>
<h2>page.url was updated {count} time(s)</h2>
21 changes: 20 additions & 1 deletion 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 @@ -428,6 +428,25 @@ test.describe('$app/state', () => {
await page.locator('button').click();
await expect(page.locator('p')).toHaveText('test');
});

test('page.url does update when used with pushState', async ({ page }) => {
await page.goto('/state/url/before');

await expect(page.locator('h1')).toHaveText('/state/url/before');
await expect(page.locator('h2')).toHaveText('page.url was updated 1 time(s)');

await page.locator('button.before').click();
await expect(page.locator('h1')).toHaveText('/state/url/before');
await expect(page.locator('h2')).toHaveText('page.url was updated 1 time(s)');

await page.locator('button.after').click();
await expect(page.locator('h1')).toHaveText('/state/url/after');
await expect(page.locator('h2')).toHaveText('page.url was updated 2 time(s)');

await page.locator('button.after').click();
await expect(page.locator('h1')).toHaveText('/state/url/after');
await expect(page.locator('h2')).toHaveText('page.url was updated 2 time(s)');
});
});

test.describe('Invalidation', () => {
Expand Down Expand Up @@ -1113,7 +1132,7 @@ test.describe('Shallow routing', () => {

await page.goForward();
expect(page.url()).toBe(`${baseURL}/shallow-routing/push-state/a`);
await expect(page.locator('h1')).toHaveText('parent');
await expect(page.locator('h1')).toHaveText('a');
await expect(page.locator('p')).toHaveText('active: true');
});

Expand Down
Loading