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

Jump to bottom

fix: prevent stale values after invalidation #11870

Merged
merged 7 commits into from
Feb 22, 2024
Merged

fix: prevent stale values after invalidation #11870

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
084bb17
fix: prevent stale values after invalidation
dummdidumm Feb 19, 2024
7c50947
same for force_invalidation, fixes #10123
dummdidumm Feb 19, 2024
9aca13f
fix
dummdidumm Feb 19, 2024
be4bc78
deduplicate
dummdidumm Feb 19, 2024
52271ad
tweak
dummdidumm Feb 19, 2024
3170f3c
document get_navigation_intent
dummdidumm Feb 21, 2024
fa16b33
Update packages/kit/test/apps/basics/test/client.test.js
dummdidumm Feb 21, 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/angry-gorillas-peel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@sveltejs/kit": patch
---

fix: prevent stale values after invalidation
29 changes: 17 additions & 12 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 @@ -308,20 +308,23 @@ async function _invalidate() {

const nav_token = (token = {});
const navigation_result = intent && (await load_route(intent));
if (nav_token !== token) return;
if (!navigation_result || nav_token !== token) return;

if (navigation_result) {
if (navigation_result.type === 'redirect') {
await _goto(new URL(navigation_result.location, current.url).href, {}, 1, nav_token);
} else {
if (navigation_result.props.page !== undefined) {
page = navigation_result.props.page;
}
root.$set(navigation_result.props);
}
if (navigation_result.type === 'redirect') {
return _goto(new URL(navigation_result.location, current.url).href, {}, 1, nav_token);
}

if (navigation_result.props.page) {
page = navigation_result.props.page;
}
current = navigation_result.state;
reset_invalidation();
root.$set(navigation_result.props);
}

function reset_invalidation() {
invalidated.length = 0;
force_invalidation = false;
}

/** @param {number} index */
Expand Down Expand Up @@ -1092,6 +1095,9 @@ async function load_root_error_page({ status, error, url, route }) {
}

/**
* Resolve the full info (which route, params, etc.) for a client-side navigation from the URL,
* taking the reroute hook into account. If this isn't a client-side-navigation (or the URL is undefined),
* returns undefined.
* @param {URL | undefined} url
* @param {boolean} invalidating
*/
Expand Down Expand Up @@ -1281,8 +1287,7 @@ async function navigate({

// reset invalidation only after a finished navigation. If there are redirects or
// additional invalidations, they should get the same invalidation treatment
invalidated.length = 0;
force_invalidation = false;
reset_invalidation();

updating = true;

Expand Down
7 changes: 7 additions & 0 deletions .../kit/test/apps/basics/src/routes/load/invalidation/invalidate-then-goto/+layout.server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/** @type {import('./$types').LayoutServerLoad} */
export function load({ depends }) {
depends('invalidate-depends:goto');
return {
layoutDate: new Date().getTime()
};
}
7 changes: 7 additions & 0 deletions ...es/kit/test/apps/basics/src/routes/load/invalidation/invalidate-then-goto/+page.server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/** @type {import('./$types').PageServerLoad} */
export function load({ url }) {
url.searchParams.get('x');
return {
pageDate: new Date().getTime()
};
}
23 changes: 23 additions & 0 deletions packages/kit/test/apps/basics/src/routes/load/invalidation/invalidate-then-goto/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<script>
import { invalidate, goto } from '$app/navigation';

/** @type {import('./$types').PageData} */
export let data;
</script>

<p class="layout">{data.layoutDate}</p>
<p class="page">{data.pageDate}</p>
<button
type="button"
class="invalidate"
on:click={() => (window.promise = invalidate('invalidate-depends:goto'))}
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
>
invalidate
</button>
<button
type="button"
class="goto"
on:click={() => (window.promise = goto('/load/invalidation/invalidate-then-goto?x'))}
>
goto
</button>
23 changes: 23 additions & 0 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 @@ -594,6 +594,29 @@
await page.locator('button').click();
await expect(page.getByText('updated')).toBeVisible();
});

test('goto after invalidation does not reset state', async ({ page }) => {
await page.goto('/load/invalidation/invalidate-then-goto');
const layout = await page.textContent('p.layout');
const _page = await page.textContent('p.page');
expect(layout).toBeDefined();
expect(_page).toBeDefined();

await page.click('button.invalidate');
await page.evaluate(() => window.promise);
const next_layout_1 = await page.textContent('p.layout');
const next_page_1 = await page.textContent('p.page');
expect(next_layout_1).not.toBe(layout);
expect(next_page_1).toBe(_page);
Comment on lines +605 to +610
Copy link
Member

Choose a reason for hiding this comment

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

I think the Playwright way to write this would be something like below where it uses its auto waiting functionality to wait until the conditions are true. I have to run, so didn't have time to check if these are the APIs with that auto waiting built-in (some of the APIs have it and some don't as I recall)

Suggested change
await page.click('button.invalidate');
await page.evaluate(() => window.promise);
const next_layout_1 = await page.textContent('p.layout');
const next_page_1 = await page.textContent('p.page');
expect(next_layout_1).not.toBe(layout);
expect(next_page_1).toBe(_page);
await page.click('button.invalidate');
expect(page.textContent('p.layout')).not.toBe(layout);
expect(page.textContent('p.page')).toBe(_page);

Copy link
Member

Choose a reason for hiding this comment

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

https://playwright.dev/docs/api/class-locatorassertions#locator-assertions-to-have-text would be the assertion to probably use here for auto-waiting.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't really use that here, because the checks that test "is this the same" could be true wrongfully if we're checking too early - which is why we do the window.promise dance.

Copy link
Member

Choose a reason for hiding this comment

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

But the first check is "are these different". If that check has passed then I think we can't be checking too early

Copy link
Member Author

Choose a reason for hiding this comment

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

Still doesn't change the fact that we gotta save the text to a variable to use it in the next comparison

Copy link
Member

Choose a reason for hiding this comment

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

But it does save you from having to do the promise dance, which would be nice to avoid


await page.click('button.goto');
await page.evaluate(() => window.promise);
const next_layout_2 = await page.textContent('p.layout');
const next_page_2 = await page.textContent('p.page');
expect(next_layout_2).toBe(next_layout_1);
expect(next_layout_2).not.toBe(layout);
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
expect(next_page_2).not.toBe(next_page_1);
Copy link
Member

Choose a reason for hiding this comment

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

you could write it like this:

Suggested change
await page.click('button.invalidate');
await page.evaluate(() => window.promise);
const next_layout_1 = await page.textContent('p.layout');
const next_page_1 = await page.textContent('p.page');
expect(next_layout_1).not.toBe(layout);
expect(next_page_1).toBe(_page);
await page.click('button.goto');
await page.evaluate(() => window.promise);
const next_layout_2 = await page.textContent('p.layout');
const next_page_2 = await page.textContent('p.page');
expect(next_layout_2).toBe(next_layout_1);
expect(next_layout_2).not.toBe(layout);
expect(next_page_2).not.toBe(next_page_1);
await page.click('button.invalidate');
expect(page.locator('p.layout').textContent()).not.toBe(layout);
expect(page.locator('p.page').textContent()).toBe(_page);
const layout2 = await page.textContent('p.layout');
const page2 = await page.textContent('p.page');
await page.click('button.goto');
expect(page.locator('p.page')).not.toBe(page2);
expect(page.locator('p.layout')).toBe(layout2);

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, it reads weird to me (first compare text content, then save that same selector to a variable). It also is missing one expect check at the end.
Also the present test mimicks the other tests in that suite, and frankly if this is the only thing holding up this PR then I'd rather discuss this separately and merge this PR because it's an important bug to fix.

Copy link
Member

Choose a reason for hiding this comment

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

If we do leave it as is then we were going to rename window.promise to something more unique, right?

});
});

test.describe('data-sveltekit attributes', () => {
Expand Down Expand Up @@ -859,7 +882,7 @@
expect(page.locator('p.loadingfail')).toBeHidden();
});

test('Catches fetch errors from server load functions (direct hit)', async ({ page }) => {

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

View workflow job for this annotation

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

flaky test: Catches fetch errors from server load functions (direct hit) 

retries: 2

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

View workflow job for this annotation

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

flaky test: Catches fetch errors from server load functions (direct hit) 

retries: 2
page.goto('/streaming/server-error');
await expect(page.locator('p.eager')).toHaveText('eager');
await expect(page.locator('p.fail')).toHaveText('fail');
Expand Down
Loading