Try to fix SPA initial behavior for 404s #4116
Conversation
🦋 Changeset detectedLatest commit: 77d5b96 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 |
|
@Rich-Harris @benmccann what do you think of this, am I missing something? |
|
I have some ideas about how this code could be cleaned up. I'll try to share something more shortly |
|
Okay, fully understand, just one of ideas, definitely not the best one. |
| @@ -294,7 +295,7 @@ export class Router { | |||
| routes: this.routes.filter(([pattern]) => pattern.test(path)), | |||
| url, | |||
| path, | |||
| initial: !this.initialized | |||
| initial: skip_browser_reload ?? !this.initialized | |||
There was a problem hiding this comment.
I haven't gotten to sit down and understand this PR yet, but I'm concerned that this variable no longer represents the initial page load, but is still being called initial. I think we'd need a better way to represent this
I sent a PR before this one to cleanup the router APIs and moved this flag to Renderer.has_hydrated: #4101
There was a problem hiding this comment.
Yeah, that makes sense
|
It was unclear to me what this is supposed to be fixing, but it looks like it's referred to in the comments of another PR (#4049 (comment)). A closed PR is a hard place to have a discussion about this. Can you open a new issue for it? |
|
Thanks. I've added a test that fails without this PR. Been thinking a little more about the circumstances in which this issue arises. We basically only need the guard when a) we're hydrating a page from a fallback |
| @@ -449,6 +450,10 @@ export class Router { | |||
| } | |||
|
|
|||
| if (details) { | |||
| if (this.is_fallback && info.url.pathname !== location.pathname) { | |||
There was a problem hiding this comment.
what is the second half of this check doing? I don't understand how an app can be initialized as a fallback/SPA and then later not become one. Maybe this variable is representing something else in this case as should have a different name?
There was a problem hiding this comment.
an SPA can have some pages prerendered. the bug we're trying to avoid is infinite reloads when the initial hydration fails (or subsequent invalidations that aren't caused by navigation), rather than preventing full page reloads across the board
| @@ -448,6 +450,10 @@ export class Router { | |||
| } | |||
|
|
|||
| if (details) { | |||
| if (this.is_fallback && info.url.pathname !== location.pathname) { | |||
There was a problem hiding this comment.
we should probably add a comment to clarify at least
| if (this.is_fallback && info.url.pathname !== location.pathname) { | |
| // if navigating to a prerendered page | |
| if (this.is_fallback && info.url.pathname !== location.pathname) { |
but another way to write this might be to move the URL check to renderer. overwriting is_fallback seems a bit funny to me because at this point it is still a fallback. it's not until after we've left and navigated to the new page that it's not a fallback
There was a problem hiding this comment.
Not sure I understand what you mean about moving the URL check to renderer. The responsibility for this stuff (to the extent that the two things are separate — I'm going to open a PR today to merge them, because it really will simplify things) lies with the router
it's not until after we've left and navigated to the new page that it's not a fallback
pushing state is navigating to the new page
There was a problem hiding this comment.
I mean you could change if (!navigation_result && this.router?.is_fallback) { to if (!navigation_result && this.router?.is_fallback && info.url.pathname !== location.pathname) {
If I understand correctly, when this is triggered we will do a server-side nav. This page is still a fallback page in my mind until the server-side nav has happened
There was a problem hiding this comment.
Actually you know what? If we were to compare url.pathname with location.pathname at that point, it would arguably be simpler to do away with the fallback/non-fallback distinction altogether and just always bail out of full page reloads if they're the same (i.e. it's not a navigation, it's a hydration/invalidation). commit inbound
There was a problem hiding this comment.
Yeah, that's simpler and should work.
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.0One of the ways to fix #4049 (comment)
we can probably completely discard that
initializationboolean in router if we decide to do this.