Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the issue with history.state resetting on refresh #12409

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yahao87
Copy link

@yahao87 yahao87 commented Jun 28, 2024

Fix the issue with history.state resetting on refresh
#11956


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.

Fix the issue with history.state resetting on refresh
Copy link

changeset-bot bot commented Jun 28, 2024

⚠️ No Changeset found

Latest commit: 79de427

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

@benmccann
Copy link
Member

this seems to be an area that's difficult to get right, so I'd really want to have a test for this one

@PatrickG
Copy link
Contributor

PatrickG commented Jun 30, 2024

The maintainers probably haven't discussed this yet, but I think this should be optional as Rich mentioned in the issue.
Also, I don't think this is the correct fix. This would only work if SSR is disabled.

@yahao87
Copy link
Author

yahao87 commented Jul 1, 2024

The maintainers probably haven't discussed this yet, but I think this should be optional as Rich mentioned in the issue. Also, I don't think this is the correct fix. This would only work if SSR is disabled.

I don't understand. Why should the framework's default behavior be to block the implementation of basic browser functionality? If a user is building a complex multi-page signup process, they can use history.state to store personal information within the history to preserve it even after a refresh without relying on a store. This history.state can store not only simple objects but also data like files selected through a form. Blocking this forces the use of localstorage.

It's natural that this doesn't work in SSR since it's a browser feature. Isn't it up to the developers using this framework to handle such implementations? The current implementation in Kit2 blocks this, taking away the user's choice.

Our service has been unable to upgrade to Kit2 since last year due to this issue.

@maj0rika
Copy link

maj0rika commented Jul 1, 2024

The current behavior of SvelteKit 2, which resets history.state on refresh, poses significant challenges for developers. When building complex multi-page signup processes, using history.state to preserve personal information even after a refresh is extremely useful. This allows developers to store data like files selected through a form without relying on local storage.

Blocking basic browser functionality imposes unnecessary constraints on developers. While it is understandable that history.state would not work in SSR (Server-Side Rendering) environments, limiting its use on the client side is inappropriate.

Moreover, the current implementation in SvelteKit 2 lacks the necessary flexibility. Instead of enforcing specific functionalities, the framework should allow developers to make their own choices. This flexibility is crucial for supporting a wide range of use cases. The inability of certain services to upgrade to SvelteKit 2 due to this restriction highlights the real problems this limitation can cause.

Overall, the current implementation of SvelteKit 2, which restricts the use of history.state, imposes significant constraints on developers and limits support for various use cases. Giving developers the flexibility to choose how they manage state is the right approach. A framework should not block basic functionality but rather empower developers to make decisions based on their specific needs.

@PatrickG
Copy link
Contributor

PatrickG commented Jul 1, 2024

The reason I mentioned SSR is because the solution for the linked issue #11956 (which is about $page.state not history.state) should work with SSR enabled as well.
I guess this PR is supposed to be a fix for #9868 (which IMO should be fixed of course).

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.

None yet

4 participants