Skip to content

Conversation

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Aug 27, 2023

Fixes an exception when back navigating before a page is fully initialized. I believe this may also fix #10512.

I'm observing that the popstate event callback raises a null pointer exception when it is triggered before current.url has been set, resulting in the following error:

TypeError
null is not an object (evaluating 'current.url.href')

From what I can follow:

  1. The page is loaded
  2. The user navigates to a second route
  3. The page is reloaded
  4. The user immediately navigates back before page initialization executes
  5. The navigation event raises an exception comparing the current and next url when restoring scroll position

When the exception is raised, the navigation code executed after scroll restoration additionally is not executed, which could be the cause of other issues mentioned in #10512

How to reproduce

I found this was easy to test in an iOS similator:

  1. Navigate to your app in the iOS simulator
  2. Initiate a web inspector session from Safari (Develop > Simulator > localhost)
  3. In the simulator, click to a second page
  4. Focus the URL address bar
  5. Move your mouse to the back button
  6. Press enter to submit the URL input
  7. Press the back button with your mouse almost immediately

While this specific case is fairly contrived, I believe it could also be responsible for errors we are seeing as a user navigates back and forward very quickly.

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

Prevents a race condition when attempting to restore the scroll
position on a `popstate` event:

1. The page is loaded
2. The user navigates to a second route
3. The page is reloaded
4. The user immediately navigates back before hydration is complete
5. The navigation event raises an exception comparing the current and
next url

When the exception is raised, the navigation code executed after
scroll restoration additionally is not executed, resulting in
additional issues.

This is easy to test in an iOS similator:

1. Navigate to your app
2. Click to a second page
3. Focus the URL address bar
4. Move your mouse to the back button
5. Press enter to submit the URL input
6. Press the back button with your mouse almost immediately

While this specific case is fairly contrived, I believe it could also
be responsible for errors we are seeing as a user navigates back and
forward very quickly.

Related to:
sveltejs#10512
@changeset-bot
Copy link

changeset-bot bot commented Aug 27, 2023

🦋 Changeset detected

Latest commit: 06a72d9

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

@nhunzaker nhunzaker changed the title Handle error when back navigating before page is initialized fix: handle error when back navigating before page is initialized Aug 27, 2023
@nhunzaker nhunzaker changed the title fix: handle error when back navigating before page is initialized fix: avoid error when back navigating before page is initialized Aug 27, 2023
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in client side navigation

2 participants