fix: reset invalid resources after successful invalidation

[Rich-Harris]

closes #10677

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:.

[changeset-bot]

🦋 Changeset detected

Latest commit: b0a6ac9

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

[gterras]

Hi, this PR is most likely the culprit for #11461 (and maybe #9355 (comment)) which triggers multiple (at least 2) versions of $page concurrently living in client even on a Svelte4 build, which is a quite sneaky bug I think more people are affected by it than reported (in our case it took several days for a user to report it despite careful inspection first).

Coincidentally or not this PR also solves #10359 which I tried to debug a few days ago (before migrating to SK2 then noticing it was solved) and came to the same conclusion as @Rich-Harris (invalidated.length = 0 in invalidate()) but it quickly appeared that it was messing with the invalidation of parent pages (ie not triggering) .

Before that PR the reset of invalidated happened after the navigation_result, see:

kit/packages/kit/src/runtime/client/client.js

Line 1063 in b2b1951

invalidated.length = 0;

)

which had the effect of calling load_route twice (which triggered the double invalidation issue) but treating invalidation properly thanks to this double trip.

What I understand of it with my limited knowledge is that now the parent "page" is never really "loaded" (in load_route()) with its dependencies since they are reset after the first load_route, leaving the parent with a non-updated "page").

I don't know the history of the file but having the reset there instead of the obvious place this PR moves it felt like there was a reason for it (maybe this one).

The fix I found (in SK1) was to check if the parent had changed before flagging the dependencies to be invalidated in has_changed() :

// has_changed()
if (!parent_changed) {
	for (const href of uses.dependencies) {
		if (invalidated.some((fn) => fn(new URL(href)))) return true;
	}
}

and let the reset where it was. I didn't do enough testing to make sure it was the right fix (although all tests seemed to pass, although at least one seems to be missing).

I could try to help if needed!

[kyngs]

The fix I found (in SK1) was to check if the parent had changed before flagging the dependencies to be invalidated in has_changed() :

// has_changed()
if (!parent_changed) {
	for (const href of uses.dependencies) {
		if (invalidated.some((fn) => fn(new URL(href)))) return true;
	}
}

and let the reset where it was. I didn't do enough testing to make sure it was the right fix (although all tests seemed to pass, although at least one seems to be missing).

@gterras Did you find any fix in SK2? The fix you sent only works if I remove the newly-added invalidated.length = 0;

[gterras]

Did you find any fix in SK2? The fix you sent only works if I remove the newly-added invalidated.length = 0;

Didn't have time to look into it yet (also not sure if a working PR would be acknowledged or awkwardly ignored by the team) but a fully working fix would definitely need more than that. Crucially it also needs least two tests first (double invalidation + unsync parent $page).

As said in my first comment the initial placement of the invalidation reset is weird enough to be there for a reason. I suspect invalidation has never really properly worked in SK history. This PR also doesn't remove the old reset which seems to be there "just in case" which seems to show invalidation logic is fully understood by no one.

I'll try to look into it soon if I find the time.