fix: fix popState race-condition when not using SSR

[aloisklink]

When loading a non-SSR page and pressing the browser's back button while the page is still loading, it's possible for current.url to still be the default value of null, which then causes the popState function to throw the following uncaught error:

Uncaught (in promise) TypeError: Cannot destructure property 'href' of 'object null' as it is null.
    at strip_hash (url.js?v=3a24e001:84:30)
    at client.js?v=3a24e001:2227:52

This is from

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

Line 2234 in e41a8e2

const is_hash_change = strip_hash(location) === strip_hash(current.url);

See #12924 for a reproduction/example videos.

---

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
  • Fixes Race-condition when pressing **Back** while the page is still loading when not using SSR #12924
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
  • I'm struggling to write an E2E test for this, so I might skip this if that's okay.

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: 758a804

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 don't understand the comment where current.url is initialized:

// @ts-ignore - we need the initial value to be null

I wonder if that's really true or why we need that to be true. Perhaps we need a separate variable to track whether the current page is initialized?

I feel like this code has gotten quite complicated and there needs to be some higher-level documentation around what states are expected at what times for what reasons, etc. I'm uncomfortable with making a change that might just be papering over an issue without really understanding what the correct fix is one way or the other. Simon or Rich might have a better understanding of this code and be able to weigh in here

[aloisklink]

I don't understand the comment where current.url is initialized:

Neither do I, to be honest! There are other places where we (I say we, but it's mainly @dummdidumm!) are checking for current.url being null.

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

Line 550 in 758a804

!current.url ||

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

Line 872 in 758a804

const url_changed = current.url ? id !== current.url.pathname + current.url.search : false;

---

Perhaps we need a separate variable to track whether the current page is initialized?

I think we could just get away with making the type in

kit/packages/kit/src/runtime/client/types.d.ts

Line 85 in f0145f1

url: URL;

URL | null. The problem is that there are many places we'd then need to add checks to see if the current page is initialized (and I think in most places, these functions are only called if the page is already initialized).

---

I'm uncomfortable with making a change that might just be papering over an issue without really understanding what the correct fix is one way or the other. Simon or Rich might have a better understanding of this code and be able to weigh in here

👍 One thing I've also noticed is that this goto is an async function, but it's missing an await (I've got no idea if this is intentional or not by @dummdidumm). I'm pretty sure adding an await here would also fix this issue, but since I'm not sure what the implications are, just "papering over" this fix seems safer.

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

Line 308 in 758a804

goto(location.href, { replaceState: true });

[dummdidumm]

thank you!

[MathiasWP]

Maybe this closes #13145?