Wait until navigation ends before updating history #3241
Conversation
🦋 Changeset detectedLatest commit: 4136461 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
❌ Deploy Preview for kit-demo failed. 🔨 Explore the source changes: 958211f 🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61d8fd3f6b034900089fdf9a |
| event.preventDefault(); | ||
| }); | ||
|
|
||
| addEventListener('popstate', (event) => { | ||
| if (event.state && this.enabled) { | ||
| const url = new URL(location.href); | ||
| this._navigate(url, event.state['sveltekit:scroll'], false, []); | ||
| this._navigate(url, event.state['sveltekit:scroll'], false, [], url.hash, null, null); |
There was a problem hiding this comment.
should we just allow these lass two parameters to be undefined so that you don't have to pass anything here?
|
I would say that I lean in favor of this change. It's taken me a couple days to comment on this as I really wanted to have time to read through and understand the various arguments and think about it. However, I think there may be a slightly different implementation that I prefer even more, which would be to update the URL in between network fetching and DOM rendering (#2870 (comment)). This is most analogous to the browser's behavior. It also satisfies quite a lot of the people who wanted the URL to update before navigation (#2917 (comment)) because a lot of those arguments seemed to be that the UI should update after the URL. It sort of seems like it could get us the best of both worlds. It's harder to figure out how to structure the code if we do that. The separation between router and renderer gets even messier. But I still think it's the best way to do it and we can figure out a refactoring later. |
That feels like the worst of all worlds to me. It would be completely imperceptible to users (the difference is basically a microtask), but would mean that we couldn't rely on page state following navigation (for example during tests). What would be the advantage? |
|
I realised there's another compelling reason to update history at the start of the navigation rather than the end — unless the app implements some sort of loading bar UI, there's no visual indication that a navigation has been accepted other than the URL changing. That can make things feel sticky. When not using client-side navigation, the browser does provide immediate UI (the reload button starts spinning), which makes this much less of a concern. (The future |
|
That's probably the best argument I've heard for doing it before. And it does look like the AppHistory API will be helpful if/when it comes around. Doing it after certainly would make testing easier as you pointed out. I'd prioritize users over ourselves though, so I guess I'm convinced on doing it before. Hopefully we can figure out the test flakiness one of these days |
|
✔️ Deploy Preview for kit-demo canceled. 🔨 Explore the source changes: 4136461 🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61df7840227c01000754070c |
|
The nice thing is that because of #3303, this is basically just a one-line change if and when we do want to do it. So there's probably no sense in leaving this PR open |
Still not sure if this is desirable or not, but I'm opening this in order to close #2917. Fixes #2870.
It would be fairly straightforward to make this configurable, but I think something this fundamental should probably be decided by the framework.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpx changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0