fix: preserve $page.state when invalidating

[PatrickG]

Fix #11783

I'm not sure if this is the correct fix or if it should be fixed in load_route()/get_navigation_result_from_branch() because this line

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

Line 988 in c749e85

// Reset `form` on navigation, but not invalidation

threw me off a bit.

Reset form on navigation, but not invalidation

This is what we need for the page state as well.

Changing this line

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

Line 531 in c749e85

state: {},

to

			state: page?.state || {},

Seems to pass all tests as well. But I'm not sure if this could lead to unexpected behavior.

---

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

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

[dummdidumm]

You hunch is right. I think the better fix is to add similar logic to the form logic: get_navigation_result_from_branch optionally accepts a state property, which defaults to the empty object. It's set nowhere except the call site where that form invalidate logic is, i.e. state: invalidating ? page.state : {}

[PatrickG]

@dummdidumm Do you mean like this?

[ConProgramming]

Just ran into this. Would love to see a fix. The native history.state still has the right data event though $page.state does not.

[ConProgramming]

This seems to resolve it #11956 (comment)
Can this be merged

[PatrickG]

This seems to resolve it #11956 (comment) Can this be merged

This PR does not fix #11956. It only fixes losing $page.state after invalidating.
An empty $page.state on the initial load is by design.
I think it should be configurable, because it does not fit all use cases.

[dummdidumm]

I think this makes sense, but will discuss with other maintainer to get agreement on whether this is a bugfix or expected behavior

[PatrickG]

With the merge of the remote functions PR, this is basically implemented in the (hacky) way I tried first.

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

Line 385 in dada03f

// This is a bit hacky but allows us not having to pass that boolean around, making things harder to reason about

But reset_page_state is only false when calling refreshAll(). IMO it should default to false when calling invalidate(...) and invalidateAll() as well, with an option to enable resetting the page state.
I'll update this PR.

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-kit-11810-svelte.vercel.app/

[Rich-Harris]

Now that this is targeting version-3 there are some changes I think we should make:

1. get rid of resetPageState — IIUC the point of this was to allow people to opt into v3 behaviour in v2, which is no longer necessary
2. it's confusing to have invalidate and invalidateAll and refreshAll. I would vote for getting rid of invalidateAll in favour of refreshAll ('refresh' is more accurate and familiar language, and mirrors the query method)
3. get rid of the { includeLoadFunctions } argument — who would ever need to refresh everything on the page except load functions? All should mean all.
4. A corollary to 2 — we should probably rename invalidate to refresh

[elliott-with-the-longest-name-on-github]

invalidate will also be confusing when we implement caching for remote functions. Typically invalidation doesn't necessitate refreshing.