fix: prevent stale values after invalidation

[dummdidumm]

Current was not updated after invalidation, which resulted in the previous values getting used on next navigation
fixes #11446
We also didn't set force_invalidate back to false
fixes #10123

---

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

• Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

[changeset-bot]

🦋 Changeset detected

Latest commit: fa16b33

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[benmccann]

I notice this used to always happen and now it only happens if there's a navigation_result. I don't understand a lot of this code to know which would be preferable. some comments could help to clarify things. e.g. I think it'd be really helpful for load_route's jsdoc to clarify what it means when it does or doesn't return a navigation result

[dummdidumm]

load_route always returns a navigation result (or will cause a hard reload), it's the intent which could be falsy. The code moved because _goto also calls invalidated.length at the end. I'm not sure it's worth it to comment this.

[benmccann]

Ah, okay. It's too bad GitHub doesn't show you the types when you hover a variable 😆 It could be worth documenting get_navigation_intent a bit then. I'm afraid I can't remember what "navigation intent" means at this point and it could be helpful to say what a falsy navigation intent represents

[benmccann]

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);

[Conduitry]

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

[dummdidumm]

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.

[benmccann]

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

[dummdidumm]

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

[benmccann]

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

[benmccann]

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);

[dummdidumm]

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.

[benmccann]

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