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.

Sign up for GitHub

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

Jump to bottom

Try to fix SPA initial behavior for 404s #4116

Merged
merged 7 commits into from
Mar 1, 2022
Merged

Try to fix SPA initial behavior for 404s #4116

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bed7e7f
init
PH4NTOMiki Feb 24, 2022
59b4e94
changeset
PH4NTOMiki Feb 24, 2022
2534fa9
add test
Rich-Harris Feb 25, 2022
b69c2d2
update lockfile
Rich-Harris Feb 28, 2022
6e491b0
only prevent reload for fallback pages
Rich-Harris Feb 28, 2022
861fb07
only render client-side 404 if pathname is unchanged
Rich-Harris Feb 28, 2022
77d5b96
Update .changeset/blue-parents-invite.md
Rich-Harris Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/blue-parents-invite.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fixes SPA mode error handling
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion packages/adapter-static/test/apps/spa/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@
"scripts": {
"dev": "../../../../kit/svelte-kit.js dev",
"build": "../../../../kit/svelte-kit.js build",
"start": "../../../../kit/svelte-kit.js start"
"start": "sirv -s 200.html build"
},
"devDependencies": {
"@sveltejs/adapter-node": "next",
"@sveltejs/kit": "next",
"sirv-cli": "^2.0.2",
"svelte": "^3.43.0"
},
"type": "module"
Expand Down
7 changes: 7 additions & 0 deletions packages/adapter-static/test/apps/spa/src/app.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// <reference types="@sveltejs/kit" />

declare namespace App {
interface Session {
count: number;
}
}
5 changes: 5 additions & 0 deletions packages/adapter-static/test/apps/spa/src/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export function getSession() {
return {
count: 0
};
}
25 changes: 25 additions & 0 deletions packages/adapter-static/test/apps/spa/src/routes/__error.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<script context="module">
/** @type {import('@sveltejs/kit').Load} */
export function load({ session }) {
return {
props: session
};
}
</script>

<script>
import { browser } from '$app/env';
import { page, session } from '$app/stores';

/** @type {number} */
export let count;

if (browser) {
$session.count += 1;
}
</script>

<h1>{$page.status}</h1>
<h2>count: {count}</h2>

<button on:click={() => ($session.count += 1)}>+1</button>
3 changes: 3 additions & 0 deletions packages/adapter-static/test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,8 @@ run('spa', (test) => {
test('renders error page for missing page', async ({ base, page }) => {
await page.goto(`${base}/nosuchpage`);
assert.equal(await page.textContent('h1'), '404');
assert.equal(await page.textContent('h2'), 'count: 1');

await page.waitForLoadState('networkidle', { timeout: 1000 });
});
});
16 changes: 8 additions & 8 deletions packages/kit/src/runtime/client/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,14 @@ export class Renderer {
const token = (this.token = {});
let navigation_result = await this._get_navigation_result(info, no_cache);

if (!navigation_result && this.router?.is_fallback) {
navigation_result = await this._load_error({
status: 404,
error: new Error(`Not found: ${info.url.pathname}`),
url: info.url
});
}

if (!navigation_result) {
location.href = info.url.href;
return;
Expand Down Expand Up @@ -507,14 +515,6 @@ export class Renderer {
);
if (result) return result;
}

if (info.initial) {
return await this._load_error({
status: 404,
error: new Error(`Not found: ${info.url.pathname}`),
url: info.url
});
}
}

/**
Expand Down
14 changes: 10 additions & 4 deletions packages/kit/src/runtime/client/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,19 @@ export class Router {
* base: string;
* routes: import('types').CSRRoute[];
* trailing_slash: import('types').TrailingSlash;
* renderer: import('./renderer').Renderer
* renderer: import('./renderer').Renderer;
* is_fallback: boolean;
* }} opts
*/
constructor({ base, routes, trailing_slash, renderer }) {
constructor({ base, routes, trailing_slash, renderer, is_fallback }) {
this.base = base;
this.routes = routes;
this.trailing_slash = trailing_slash;
/** Keeps tracks of multiple navigations caused by redirects during rendering */
this.navigating = 0;

this.is_fallback = is_fallback;

/** @type {import('./renderer').Renderer} */
this.renderer = renderer;
renderer.router = this;
Expand Down Expand Up @@ -293,8 +296,7 @@ export class Router {
id: url.pathname + url.search,
routes: this.routes.filter(([pattern]) => pattern.test(path)),
url,
path,
initial: !this.initialized
path
};
}
}
Expand Down Expand Up @@ -448,6 +450,10 @@ export class Router {
}

if (details) {
if (this.is_fallback && info.url.pathname !== location.pathname) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

we should probably add a comment to clarify at least

Suggested change
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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's simpler and should work.

this.is_fallback = false;
}

const change = details.replaceState ? 0 : 1;
details.state['sveltekit:index'] = this.current_history_index += change;
history[details.replaceState ? 'replaceState' : 'pushState'](details.state, '', info.url);
Expand Down
15 changes: 13 additions & 2 deletions packages/kit/src/runtime/client/start.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { set_paths } from '../paths.js';
* session: any;
* route: boolean;
* spa: boolean;
* is_fallback: boolean;
* trailing_slash: import('types').TrailingSlash;
* hydrate: {
* status: number;
Expand All @@ -25,7 +26,16 @@ import { set_paths } from '../paths.js';
* };
* }} opts
*/
export async function start({ paths, target, session, route, spa, trailing_slash, hydrate }) {
export async function start({
paths,
target,
session,
route,
spa,
is_fallback,
trailing_slash,
hydrate
}) {
const renderer = new Renderer({
Root,
fallback,
Expand All @@ -38,7 +48,8 @@ export async function start({ paths, target, session, route, spa, trailing_slash
base: paths.base,
routes,
trailing_slash,
renderer
renderer,
is_fallback
})
: null;

Expand Down
1 change: 0 additions & 1 deletion packages/kit/src/runtime/client/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ export type NavigationInfo = {
routes: CSRRoute[];
url: URL;
path: string;
initial: boolean;
};

export type NavigationCandidate = {
Expand Down
1 change: 1 addition & 0 deletions packages/kit/src/runtime/server/page/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ export async function render_response({
})},
route: ${!!page_config.router},
spa: ${!resolve_opts.ssr},
is_fallback: ${!!state.prerender?.fallback},
trailing_slash: ${s(options.trailing_slash)},
hydrate: ${resolve_opts.ssr && page_config.hydrate ? `{
status: ${status},
Expand Down
89 changes: 77 additions & 12 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.