From dc8ec1e44582969d5db4e0d223cf5429be374bdb Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Mon, 6 Jan 2025 18:33:29 +0800 Subject: [PATCH] fix: correctly notify page store subscribers (#13205) fixes #13200 This PR ensures page store subscribers are correctly notified by creating a new object whenever we update the page store. I'm not sure why but when the same page object is used, it doesn't notify subscribers. There was also another bug where a popstate event would cause subscribers to get notified twice because we were calling root.$set({ page }) in addition to stores.page.notify() when updating the URL. --- .changeset/dirty-bees-act.md | 5 ++ packages/kit/src/runtime/client/client.js | 60 ++++++++++++------- .../src/routes/store/subscribe/+layout.svelte | 47 +++++++++++++++ .../src/routes/store/subscribe/+page.svelte | 0 .../kit/test/apps/basics/test/client.test.js | 54 +++++++++++++++++ 5 files changed, 146 insertions(+), 20 deletions(-) create mode 100644 .changeset/dirty-bees-act.md create mode 100644 packages/kit/test/apps/basics/src/routes/store/subscribe/+layout.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/store/subscribe/+page.svelte diff --git a/.changeset/dirty-bees-act.md b/.changeset/dirty-bees-act.md new file mode 100644 index 000000000000..94a55f6070de --- /dev/null +++ b/.changeset/dirty-bees-act.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: correctly notify page store subscribers diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 9f3451a68d51..369ffa9e25aa 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -520,7 +520,7 @@ function get_navigation_result_from_branch({ url, params, branch, status, error, props: { // @ts-ignore Somehow it's getting SvelteComponent and SvelteComponentDev mixed up constructors: compact(branch).map((branch_node) => branch_node.node.component), - page + page: clone_page(page) } }; @@ -861,7 +861,10 @@ function preload_error({ error, url, route, params }) { params, branch: [] }, - props: { page, constructors: [] } + props: { + page: clone_page(page), + constructors: [] + } }; } @@ -1972,7 +1975,10 @@ export function pushState(url, state) { has_navigated = true; page.state = state; - root.$set({ page }); + root.$set({ + // we need to assign a new page object so that subscribers are correctly notified + page: clone_page(page) + }); clear_onward_history(current_history_index, current_navigation_index); } @@ -2013,7 +2019,9 @@ export function replaceState(url, state) { history.replaceState(opts, '', resolve_url(url)); page.state = state; - root.$set({ page }); + root.$set({ + page: clone_page(page) + }); } /** @@ -2064,7 +2072,7 @@ export async function applyAction(result) { // this brings Svelte's view of the world in line with SvelteKit's // after use:enhance reset the form.... form: null, - page + page: clone_page(page) }); // ...so that setting the `form` prop takes effect and isn't ignored @@ -2317,16 +2325,15 @@ function _start_router() { // This happens with hash links and `pushState`/`replaceState`. The // exception is if we haven't navigated yet, since we could have // got here after a modal navigation then a reload + if (state !== page.state) { + page.state = state; + } + update_url(url); scroll_positions[current_history_index] = scroll_state(); if (scroll) scrollTo(scroll.x, scroll.y); - if (state !== page.state) { - page.state = state; - root.$set({ page }); - } - current_history_index = history_index; return; } @@ -2409,16 +2416,7 @@ function _start_router() { */ function update_url(url) { current.url = page.url = url; - stores.page.set({ - data: page.data, - error: page.error, - form: page.form, - params: page.params, - route: page.route, - state: page.state, - status: page.status, - url - }); + stores.page.set(clone_page(page)); stores.page.notify(); } } @@ -2759,6 +2757,28 @@ function create_navigation(current, intent, url, type) { }; } +/** + * TODO: remove this in 3.0 when the page store is also removed + * + * We need to assign a new page object so that subscribers are correctly notified. + * However, spreading `{ ...page }` returns an empty object so we manually + * assign to each property instead. + * + * @param {import('@sveltejs/kit').Page} page + */ +function clone_page(page) { + return { + data: page.data, + error: page.error, + form: page.form, + params: page.params, + route: page.route, + state: page.state, + status: page.status, + url: page.url + }; +} + if (DEV) { // Nasty hack to silence harmless warnings the user can do nothing about const console_warn = console.warn; diff --git a/packages/kit/test/apps/basics/src/routes/store/subscribe/+layout.svelte b/packages/kit/test/apps/basics/src/routes/store/subscribe/+layout.svelte new file mode 100644 index 000000000000..4712a7b94581 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/store/subscribe/+layout.svelte @@ -0,0 +1,47 @@ + + +

{count}

+ + + + + + + + + + + +{@render children()} diff --git a/packages/kit/test/apps/basics/src/routes/store/subscribe/+page.svelte b/packages/kit/test/apps/basics/src/routes/store/subscribe/+page.svelte new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js index 313d2d5cb75d..6bdad95dc0fa 100644 --- a/packages/kit/test/apps/basics/test/client.test.js +++ b/packages/kit/test/apps/basics/test/client.test.js @@ -394,6 +394,60 @@ test.describe('$app/stores', () => { await app.goto('/store/data/store-update/same-keys'); await expect(page.locator('p')).toHaveText('$page.data was updated 1 time(s)'); }); + + test('page subscribers are notified when invalidate is called', async ({ page }) => { + await page.goto('/store/subscribe'); + await expect(page.locator('p')).toHaveText('1'); + await page.locator('button', { hasText: 'invalidate' }).click(); + await expect(page.locator('p')).toHaveText('2'); + await page.locator('button', { hasText: 'invalidate' }).click(); + await expect(page.locator('p')).toHaveText('3'); + }); + + test('page subscribers are notified when replaceState is called', async ({ page }) => { + await page.goto('/store/subscribe'); + await expect(page.locator('p')).toHaveText('1'); + await page.locator('button', { hasText: 'replaceState' }).click(); + await expect(page.locator('p')).toHaveText('2'); + await page.locator('button', { hasText: 'replaceState' }).click(); + await expect(page.locator('p')).toHaveText('3'); + }); + + test('page subscribers are notified when pushState is called', async ({ page }) => { + await page.goto('/store/subscribe'); + await expect(page.locator('p')).toHaveText('1'); + await page.locator('button', { hasText: 'pushState' }).click(); + await expect(page.locator('p')).toHaveText('2'); + await page.locator('button', { hasText: 'pushState' }).click(); + await expect(page.locator('p')).toHaveText('3'); + }); + + test('page subscribers are notified when goto is called', async ({ page }) => { + await page.goto('/store/subscribe'); + await expect(page.locator('p')).toHaveText('1'); + await page.locator('button', { hasText: 'goto' }).click(); + await expect(page.locator('p')).toHaveText('2'); + await page.locator('button', { hasText: 'goto' }).click(); + await expect(page.locator('p')).toHaveText('3'); + }); + + test('page subscribers are notified when applyAction is called', async ({ page }) => { + await page.goto('/store/subscribe'); + await expect(page.locator('p')).toHaveText('1'); + await page.locator('button', { hasText: 'applyAction' }).click(); + await expect(page.locator('p')).toHaveText('2'); + await page.locator('button', { hasText: 'applyAction' }).click(); + await expect(page.locator('p')).toHaveText('3'); + }); + + test('page subscribers are notified only once after popstate', async ({ page }) => { + await page.goto('/store/subscribe'); + await expect(page.locator('p')).toHaveText('1'); + await page.locator('button', { hasText: 'pushState' }).click(); + await expect(page.locator('p')).toHaveText('2'); + await page.goBack(); + await expect(page.locator('p')).toHaveText('3'); + }); }); test.describe('$app/state', () => {