fix: invalidate parent load functions #11819

gterras · 2024-02-09T14:28:45Z

also probably
#11635
#11663
#11446
#10209
#10123
#10457
#10359
#11745

Most likely #11268 introduced this behavior that is a blocker preventing an upgrade to SvelteKit2 for any project using "parent invalidation".

Draft status as it needs 2 tests:

double invalidation (Invalidate + Goto triggers load() twice with custom identifier #10359), is it technically possible to test this?
parent invalidation synchronization

I would like to dig into the tests but could someone confirm this PR is on the right tracks?

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

[ x] 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
[ x] This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

[ x] 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

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

changeset-bot · 2024-02-09T14:28:48Z

⚠️ No Changeset found

Latest commit: e6d103a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

kyngs · 2024-02-09T16:02:27Z

I've tested this and can confirm that it fixes issue #11696

dummdidumm

Thanks for making a start at this - though I'm not sure if this looks correct yet. In general the whole "when things are invalidated / reset" is hella hard to figure out. I have to think this through next week.

dummdidumm · 2024-02-09T16:08:04Z

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

@@ -753,8 +752,10 @@ function has_changed(
 		if (params[param] !== current.params[param]) return true;
 	}

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


Why would this logic only run if the parent hasn't changed? Wouldn't this result in false negatives?

What I understand of the whole thing is that every "parent routes" needs to go through the whole invalidation loop else their version of data is not updated. The logic in the file starts with the current page, then up a level until there is none.

The fix in #11268 would strictly break this loop after first iteration, so parents are never actually loaded.

Having !parent_changed there is a mean to ensure this is not the first iteration of the loop (meaning it's a parent), so it's data isn't actually marked for invalidation / invalidated (no need) but every routes still go through a invalidation process (so they get the updated data).

So basically there is a invalidation process that do several things (invalidate the data and assign this data to the route being invalidated), and every segment of the page need to go through this process to receive the proper data, but only the current page segment have to actually invalidate data.

If you prevent parent segments from going into this loop they never get the updated data.

dummdidumm · 2024-02-09T16:08:37Z

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

@@ -318,7 +318,6 @@ async function _invalidate() {
 		}
 	}

-	invalidated.length = 0;


This would probably bring back #10677?

Most likely, I see tests failing in the PR but can't find a useful error

It looks like #11268 did not fix issue #10677 completely. Walking through the instructions in https://github.com/davideaster/sveltekit-invalidations I can still reproduce the error in the last step on the latest commit of sveltekit.

However, when I check out this PR, I can confirm that it brings back the first error from the reproduction repo I've linked above. The PR fixed that first error (invalidate on a non-depends resource following invalidate with depends).

gterras · 2024-02-09T16:31:24Z

Thanks for making a start at this - though I'm not sure if this looks correct yet. In general the whole "when things are invalidated / reset" is hella hard to figure out. I have to think this through next week.

Thanks for answering 🙏 Yes it really seems to be a tough topic and really hard to tackle without additional tests, I can try to add failing ones if you need it. This one especially

double invalidation (#10359), is it technically possible to test this?

Starting from this one could be a softer approach since it's simpler but have similar roots.

benmccann · 2024-02-17T06:11:42Z

Yes it really seems to be a tough topic and really hard to tackle without additional tests, I can try to add failing ones if you need it.

That would be a huge help. I'm on vacation for the next week, so unfortunately not likely to make progress on this personally in the short-term. Tests would help expedite things so that some progress is made in the meantime

dummdidumm · 2024-02-19T12:44:22Z

Found the culprit, closing in favor of #11870 - thank you for assembling that list of issues! Will go through that to see if that fix also fixes one of the other ones.

fix: invalidate all current load functions

e6d103a

gterras mentioned this pull request Feb 9, 2024

Regression in v2: parent() returns old data even after the parent was invalidated using invalidate() #11696

Closed

dummdidumm reviewed Feb 9, 2024

View reviewed changes

dummdidumm closed this Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: invalidate parent load functions #11819

fix: invalidate parent load functions #11819

gterras commented Feb 9, 2024 •

edited

Loading

changeset-bot bot commented Feb 9, 2024

kyngs commented Feb 9, 2024

dummdidumm left a comment

dummdidumm Feb 9, 2024

gterras Feb 9, 2024 •

edited

Loading

dummdidumm Feb 9, 2024

gterras Feb 9, 2024 •

edited

Loading

kyngs Feb 9, 2024 •

edited

Loading

gterras commented Feb 9, 2024 •

edited

Loading

benmccann commented Feb 17, 2024

dummdidumm commented Feb 19, 2024

fix: invalidate parent load functions #11819

fix: invalidate parent load functions #11819

Conversation

gterras commented Feb 9, 2024 • edited Loading

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

Tests

Changesets

Edits

changeset-bot bot commented Feb 9, 2024

⚠️ No Changeset found

kyngs commented Feb 9, 2024

dummdidumm left a comment

Choose a reason for hiding this comment

dummdidumm Feb 9, 2024

Choose a reason for hiding this comment

gterras Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

dummdidumm Feb 9, 2024

Choose a reason for hiding this comment

gterras Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

kyngs Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

gterras commented Feb 9, 2024 • edited Loading

benmccann commented Feb 17, 2024

dummdidumm commented Feb 19, 2024

gterras commented Feb 9, 2024 •

edited

Loading

gterras Feb 9, 2024 •

edited

Loading

gterras Feb 9, 2024 •

edited

Loading

kyngs Feb 9, 2024 •

edited

Loading

gterras commented Feb 9, 2024 •

edited

Loading