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

Remove fallthrough #4330

Merged
merged 9 commits into from
Mar 16, 2022
Merged

Remove fallthrough #4330

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5799667
remove fallthrough
Rich-Harris Mar 15, 2022
1e9e2ca
changeset
Rich-Harris Mar 15, 2022
1db07a4
remove fallthrough documentation
Rich-Harris Mar 15, 2022
5fa067b
tweak docs
Rich-Harris Mar 15, 2022
ca3d714
simplify
Rich-Harris Mar 15, 2022
705721b
simplify
Rich-Harris Mar 15, 2022
7a56ac2
simplify a tiny bit
Rich-Harris Mar 15, 2022
eb77dfd
lint
Rich-Harris Mar 15, 2022
aa415d9
oops
Rich-Harris Mar 15, 2022
File filter

Filter by extension

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

[breaking] remove fallthrough routes
47 changes: 3 additions & 44 deletions documentation/docs/01-routing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,11 +323,10 @@ A route can have multiple dynamic parameters, for example `src/routes/[category]
It's possible for multiple routes to match a given path. For example each of these routes would match `/foo-abc`:

```bash
src/routes/[...catchall].svelte
Copy link
Member

Choose a reason for hiding this comment

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

should you be allowed to have [...catchall].svelte and [b].svelte? It seems like [b].svelte will never be called, so maybe this should throw an error during build

Copy link
Member

Choose a reason for hiding this comment

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

They would both be allowed once we implement part 2 of this with route validators, right? (And that is going to happen immediately after this PR.)

Copy link
Member Author

Choose a reason for hiding this comment

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

[...catchall] is a rest parameter. regardless of part 2, it will match /things/b/will/not/match. We could throw an error for [a].js alongside [b].svelte, but I think we can do that separately to avoid bloating this PR

Copy link
Member

Choose a reason for hiding this comment

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

I assumed this was the section where we showed the priority order of routes (GitHub collapsed the context around this). It looks like [..catchall].svelte is still last in the priority ordering. I wonder if we should show it last here as well for consistency?

src/routes/[a].js
src/routes/[b].svelte
src/routes/[c].svelte
src/routes/[...catchall].svelte
src/routes/foo-[bar].svelte
src/routes/foo-[c].svelte
```

SvelteKit needs to know which route is being requested. To do so, it sorts them according to the following rules...
Expand All @@ -340,48 +339,8 @@ SvelteKit needs to know which route is being requested. To do so, it sorts them
...resulting in this ordering, meaning that `/foo-abc` will invoke `src/routes/foo-[bar].svelte` rather than a less specific route:

```bash
src/routes/foo-[bar].svelte
src/routes/foo-[c].svelte
src/routes/[a].js
src/routes/[b].svelte
src/routes/[c].svelte
src/routes/[...catchall].svelte
```

#### Fallthrough routes

In rare cases, the ordering above might not be what you want for a given path. For example, perhaps `/foo-abc` should resolve to `src/routes/foo-[bar].svelte`, but `/foo-def` should resolve to `src/routes/[b].svelte`.

Higher priority routes can _fall through_ to lower priority routes by returning `{ fallthrough: true }`, either from `load` (for pages) or a request handler (for endpoints):

```svelte
/// file: src/routes/foo-[bar].svelte
<script context="module">
export function load({ params }) {
if (params.bar === 'def') {
return { fallthrough: true };
}

// ...
}
</script>
```

```js
/// file: src/routes/[a].js

// @filename: [a].d.ts
import type { RequestHandler as GenericRequestHandler } from '@sveltejs/kit';
export type RequestHandler<Body = any> = GenericRequestHandler<{ a: string }, Body>;

// @filename: index.js
// @errors: 2366
// ---cut---
/** @type {import('./[a]').RequestHandler} */
export function get({ params }) {
if (params.a === 'foo-def') {
return { fallthrough: true };
}

// ...
}
```
74 changes: 23 additions & 51 deletions packages/kit/src/runtime/client/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export function create_client({ target, session, base, trailing_slash }) {

const intent = get_navigation_intent(url);

load_cache.promise = get_navigation_result(intent, false);
load_cache.promise = load_route(intent, false);
load_cache.id = intent.id;

return load_cache.promise;
Expand All @@ -191,7 +191,7 @@ export function create_client({ target, session, base, trailing_slash }) {
*/
async function update(intent, redirect_chain, no_cache, opts) {
const current_token = (token = {});
let navigation_result = await get_navigation_result(intent, no_cache);
let navigation_result = await load_route(intent, no_cache);

if (!navigation_result && intent.url.pathname === location.pathname) {
// this could happen in SPA fallback mode if the user navigated to
Expand Down Expand Up @@ -341,36 +341,6 @@ export function create_client({ target, session, base, trailing_slash }) {
}
}

/**
* @param {import('./types').NavigationIntent} intent
* @param {boolean} no_cache
*/
async function get_navigation_result(intent, no_cache) {
if (load_cache.id === intent.id && load_cache.promise) {
return load_cache.promise;
}

for (let i = 0; i < intent.routes.length; i += 1) {
const route = intent.routes[i];

// load code for subsequent routes immediately, if they are as
// likely to match the current path/query as the current one
let j = i + 1;
while (j < intent.routes.length) {
const next = intent.routes[j];
if (next[0].toString() === route[0].toString()) {
next[1].forEach((loader) => loader());
j += 1;
} else {
break;
}
}

const result = await load_route(route, intent, no_cache);
if (result) return result;
}
}

/**
*
* @param {{
Expand Down Expand Up @@ -557,11 +527,16 @@ export function create_client({ target, session, base, trailing_slash }) {
}

/**
* @param {import('types').CSRRoute} route
* @param {import('./types').NavigationIntent} intent
* @param {boolean} no_cache
*/
async function load_route(route, { id, url, path, routes }, no_cache) {
async function load_route({ id, url, path, route }, no_cache) {
if (!route) return;

if (load_cache.id === id && load_cache.promise) {
return load_cache.promise;
}

if (!no_cache) {
const cached = cache.get(id);
if (cached) return cached;
Expand Down Expand Up @@ -625,7 +600,7 @@ export function create_client({ target, session, base, trailing_slash }) {
`${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`,
{
headers: {
'x-sveltekit-load': /** @type {string} */ (shadow_key)
'x-sveltekit-load': 'true'
}
}
);
Expand All @@ -641,15 +616,7 @@ export function create_client({ target, session, base, trailing_slash }) {
};
}

if (res.status === 204) {
if (route !== routes[routes.length - 1]) {
// fallthrough
return;
}
props = {};
} else {
props = await res.json();
}
props = res.status === 204 ? {} : await res.json();
} else {
status = res.status;
error = new Error('Failed to load data');
Expand All @@ -672,9 +639,13 @@ export function create_client({ target, session, base, trailing_slash }) {
}

if (node.loaded) {
// TODO remove for 1.9
// @ts-expect-error
if (node.loaded.fallthrough) {
throw new Error('fallthrough is no longer supported');
return;
}

if (node.loaded.error) {
status = node.loaded.status;
error = node.loaded.error;
Expand Down Expand Up @@ -811,6 +782,7 @@ export function create_client({ target, session, base, trailing_slash }) {

/** @param {URL} url */
function owns(url) {
// TODO now that we've got rid of fallthrough, check against routes immediately
return url.origin === location.origin && url.pathname.startsWith(base);
}

Expand All @@ -821,7 +793,7 @@ export function create_client({ target, session, base, trailing_slash }) {
/** @type {import('./types').NavigationIntent} */
const intent = {
id: url.pathname + url.search,
routes: routes.filter(([pattern]) => pattern.test(path)),
route: routes.find(([pattern]) => pattern.test(path)),
url,
path
};
Expand Down Expand Up @@ -860,14 +832,14 @@ export function create_client({ target, session, base, trailing_slash }) {
return;
}

if (!owns(url)) {
const pathname = normalize_path(url.pathname, trailing_slash);
const normalized = new URL(url.origin + pathname + url.search + url.hash);

if (!owns(normalized)) {
await native_navigation(url);
}

const pathname = normalize_path(url.pathname, trailing_slash);
url = new URL(url.origin + pathname + url.search + url.hash);

const intent = get_navigation_intent(url);
const intent = get_navigation_intent(normalized);

update_scroll_positions(current_history_index);

Expand Down Expand Up @@ -896,7 +868,7 @@ export function create_client({ target, session, base, trailing_slash }) {
if (navigating_token !== current_navigating_token) return;

if (!navigating) {
const navigation = { from, to: url };
const navigation = { from, to: normalized };
callbacks.after_navigate.forEach((fn) => fn(navigation));

stores.navigating.set(null);
Expand Down
4 changes: 2 additions & 2 deletions 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 @@ -38,9 +38,9 @@ export type NavigationIntent = {
*/
path: string;
/**
* The routes that could satisfy this navigation intent
* The route that matches `path`
*/
routes: CSRRoute[];
route: CSRRoute | undefined; // TODO i'm pretty sure we can make this required, and simplify some stuff
/**
* The destination URL
*/
Expand Down
3 changes: 3 additions & 0 deletions packages/kit/src/runtime/server/endpoint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ export async function render_endpoint(event, mod) {
return error(`${preface}: expected an object, got ${typeof response}`);
}

// TODO remove for 1.9
// @ts-expect-error
if (response.fallthrough) {
throw new Error('fallthrough is no longer supported');
return;
}

Expand Down
20 changes: 4 additions & 16 deletions packages/kit/src/runtime/server/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,17 +165,7 @@ export async function respond(request, options, state) {
event.url = new URL(event.url.origin + normalized + event.url.search);
}

// `key` will be set if this request came from a client-side navigation
// to a page with a matching endpoint
const key = request.headers.get('x-sveltekit-load');

for (const route of options.manifest._.routes) {
if (key) {
// client is requesting data for a specific endpoint
if (route.type !== 'page') continue;
if (route.key !== key) continue;
}

const match = route.pattern.exec(decoded);
if (!match) continue;

Expand All @@ -188,7 +178,7 @@ export async function respond(request, options, state) {
response = await render_endpoint(event, await route.shadow());

// loading data for a client-side transition is a special case
if (key) {
if (request.headers.has('x-sveltekit-load')) {
if (response) {
// since redirects are opaque to the browser, we need to repackage
// 3xx responses as 200s with a custom header
Expand All @@ -205,12 +195,8 @@ export async function respond(request, options, state) {
}
}
} else {
// fallthrough
response = new Response(undefined, {
status: 204,
headers: {
'content-type': 'application/json'
}
status: 204
});
}
}
Expand Down Expand Up @@ -257,6 +243,8 @@ export async function respond(request, options, state) {

return response;
}

break;
dominikg marked this conversation as resolved.
Show resolved Hide resolved
}

// if this request came direct from the user, rather than
Expand Down
29 changes: 19 additions & 10 deletions packages/kit/src/runtime/server/page/load_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { coalesce_to_error } from '../../../utils/error.js';
* status?: number;
* error?: Error;
* }} opts
* @returns {Promise<import('./types').Loaded | undefined>} undefined for fallthrough
* @returns {Promise<import('./types').Loaded>}
*/
export async function load_node({
event,
Expand Down Expand Up @@ -50,7 +50,7 @@ export async function load_node({
*/
let set_cookie_headers = [];

/** @type {import('types').Either<import('types').Fallthrough, import('types').LoadOutput>} */
/** @type {import('types').LoadOutput} */
let loaded;

/** @type {import('types').ShadowData} */
Expand All @@ -63,8 +63,6 @@ export async function load_node({
)
: {};

if (shadow.fallthrough) return;

if (shadow.cookies) {
set_cookie_headers.push(...shadow.cookies);
}
Expand Down Expand Up @@ -325,8 +323,15 @@ export async function load_node({
loaded = await module.load.call(null, load_input);

if (!loaded) {
// TODO do we still want to enforce this now that there's no fallthrough?
throw new Error(`load function must return a value${options.dev ? ` (${node.entry})` : ''}`);
}

// TODO remove for 1.9
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
// @ts-expect-error
if (loaded.fallthrough) {
throw new Error('fallthrough is no longer supported');
}
} else if (shadow.body) {
loaded = {
props: shadow.body
Expand All @@ -335,10 +340,6 @@ export async function load_node({
loaded = {};
}

if (loaded.fallthrough && !is_error) {
return;
}

// generate __data.json files when prerendering
if (shadow.body && state.prerender) {
const pathname = `${event.url.pathname.replace(/\/$/, '')}/__data.json`;
Expand Down Expand Up @@ -401,7 +402,11 @@ async function load_shadow_data(route, event, options, prerender) {
if (!is_get) {
const result = await handler(event);

if (result.fallthrough) return result;
// TODO remove for 1.9
// @ts-expect-error
if (result.fallthrough) {
throw new Error('fallthrough is no longer supported');
}

const { status, headers, body } = validate_shadow_output(result);
data.status = status;
Expand All @@ -426,7 +431,11 @@ async function load_shadow_data(route, event, options, prerender) {
if (get) {
const result = await get(event);

if (result.fallthrough) return result;
// TODO remove for 1.9
// @ts-expect-error
if (result.fallthrough) {
throw new Error('fallthrough is no longer supported');
}

const { status, headers, body } = validate_shadow_output(result);
add_cookies(/** @type {string[]} */ (data.cookies), headers);
Expand Down
5 changes: 0 additions & 5 deletions packages/kit/test/apps/basics/src/routes/endpoint-output/empty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,3 @@
export function get() {
return {};
}

/** @type {import('@sveltejs/kit').RequestHandler} */
export function del() {
return { fallthrough: true };
}
Loading