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 Clashing endpoint-powered pages break route sorting(#4038) #4139

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion packages/kit/src/core/create_app/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ function generate_client_manifest(manifest_data, base) {

// optional items
if (params || route.shadow) tuple.push(params || 'null');
if (route.shadow) tuple.push('1');

if (route.shadow) tuple.push(`'${route.key}'`);
return `// ${route.a[route.a.length - 1]}\n\t\t[${tuple.join(', ')}]`;
}
})
Expand Down
33 changes: 18 additions & 15 deletions packages/kit/src/core/create_manifest_data/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,22 +261,25 @@ export default function create_manifest_data({

walk(config.kit.files.routes, [], [], [], [layout], [error]);

// merge matching page/endpoint pairs into shadowed pages
let i = routes.length;
while (i--) {
const route = routes[i];
const prev = routes[i - 1];

if (prev && prev.key === route.key) {
if (prev.type !== 'endpoint' || route.type !== 'page') {
const relative = path.relative(cwd, path.resolve(config.kit.files.routes, prev.key));
throw new Error(`Duplicate route files: ${relative}`);
}

route.shadow = prev.file;
routes.splice(--i, 1);
const pages = new Map();
/** @type {number[]} */
const endpoints = [];
routes.forEach((route, i) => {
if (route.type === 'page') {
pages.set(route.key, route);
} else {
endpoints.unshift(i);
}
}
});
endpoints.forEach((i) => {
const endpoint = routes[i];
const page = pages.get(endpoint.key);
if (page) {
// @ts-ignore
page.shadow = endpoint.file;
routes.splice(i, 1);
}
});

const assets = fs.existsSync(config.kit.files.assets)
? list_files({ config, dir: config.kit.files.assets, path: '' })
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/src/core/dev/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export async function create_plugin(config, cwd) {
routes: manifest_data.routes.map((route) => {
if (route.type === 'page') {
return {
key: route.key,
type: 'page',
pattern: route.pattern,
params: get_params(route.params),
Expand All @@ -119,7 +120,6 @@ export async function create_plugin(config, cwd) {
b: route.b.map((id) => manifest_data.components.indexOf(id))
};
}

return {
type: 'endpoint',
pattern: route.pattern,
Expand Down
1 change: 1 addition & 0 deletions packages/kit/src/core/generate_manifest/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export function generate_manifest(
${routes.map(route => {
if (route.type === 'page') {
return `{
key: '${route.key}',
type: 'page',
pattern: ${route.pattern},
params: ${get_params(route.params)},
Expand Down
70 changes: 44 additions & 26 deletions packages/kit/src/runtime/client/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -724,17 +724,19 @@ export class Renderer {
/**
* @param {import('./types').NavigationCandidate} selected
* @param {boolean} no_cache
* @param {string} [fallthrough_target]
* @param {Record<string, any>} [fallthrough_props]
* @returns {Promise<import('./types').NavigationResult | undefined>} undefined if fallthrough
*/
async _load({ route, info: { url, path } }, no_cache) {
async _load({ route, info }, no_cache, fallthrough_target, fallthrough_props) {
const { url, path, routes } = info;
const key = url.pathname + url.search;

if (!no_cache) {
const cached = this.cache.get(key);
if (cached) return cached;
}

const [pattern, a, b, get_params, has_shadow] = route;
const [pattern, a, b, get_params, shadow_key] = route;
const params = get_params
? // the pattern is for the route which we've already matched to this path
get_params(/** @type {RegExpExecArray} */ (pattern.exec(path)))
Expand Down Expand Up @@ -785,33 +787,49 @@ export class Renderer {
/** @type {Record<string, any>} */
let props = {};

const is_shadow_page = has_shadow && i === a.length - 1;
const is_shadow_page = shadow_key !== undefined && i === a.length - 1;

if (is_shadow_page) {
const res = await fetch(
`${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`,
{
headers: {
'x-sveltekit-load': 'true'
if (fallthrough_target !== undefined && shadow_key === fallthrough_target) {
if (fallthrough_props) props = fallthrough_props;
} else {
const res = await fetch(
`${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`,
{
// @ts-ignore
headers: {
'x-sveltekit-load': shadow_key
}
}
);
if (res.ok) {
const redirect = res.headers.get('x-sveltekit-location');
const route_index = res.headers.get('x-sveltekit-load');
if (redirect) {
return {
redirect,
props: {},
state: this.current
};
}
// '.' means fallthrough from server-side not match any router
if (route_index === '-1') return;
props = res.status === 204 ? {} : await res.json();
if (route_index) {
const next_route = routes[+route_index];
if (next_route) {
return await this._load(
{ route: next_route, info },
no_cache,
next_route[4],
props
);
}
}
} else {
status = res.status;
error = new Error('Failed to load data');
}
);

if (res.ok) {
const redirect = res.headers.get('x-sveltekit-location');

if (redirect) {
return {
redirect,
props: {},
state: this.current
};
}

props = await res.json();
} else {
status = res.status;
error = new Error('Failed to load data');
}
}

Expand Down
73 changes: 52 additions & 21 deletions packages/kit/src/runtime/server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,26 +151,29 @@ export async function respond(request, options, state = {}) {
event.url = new URL(event.url.origin + normalized + event.url.search);
}

const shadow_key = request.headers.get('x-sveltekit-load');
let from_fallthrough = false;
let route_index = -1;

for (const route of options.manifest._.routes) {
const match = route.pattern.exec(decoded);
if (!match) continue;

event.params = route.params ? decode_params(route.params(match)) : {};

/** @type {Response | undefined} */
let response;

if (is_data_request && route.type === 'page' && route.shadow) {
response = await render_endpoint(event, await route.shadow());

// loading data for a client-side transition is a special case
if (request.headers.get('x-sveltekit-load') === 'true') {
const is_page = route.type === 'page';
if (is_page) route_index++;
if (is_data_request && is_page && route.shadow) {
if (!from_fallthrough && shadow_key && shadow_key !== route.key) continue;
if (route.shadow) {
response = await render_endpoint(event, await route.shadow());
// loading data for a client-side transition is a special case
if (response) {
// since redirects are opaque to the browser, we need to repackage
// 3xx responses as 200s with a custom header
if (response.status >= 300 && response.status < 400) {
const location = response.headers.get('location');

if (location) {
const headers = new Headers(response.headers);
headers.set('x-sveltekit-location', location);
Expand All @@ -180,21 +183,39 @@ export async function respond(request, options, state = {}) {
});
}
}
if (from_fallthrough && response.ok) {
const headers = new Headers(response.headers);
// client-site will fallthrough to the route
headers.set('x-sveltekit-load', `${route_index}`);
response = new Response(response.body, {
status: response.status,
headers
});
}
} else {
// TODO ideally, the client wouldn't request this data
// in the first place (at least in production)
response = new Response('{}', {
headers: {
'content-type': 'application/json'
}
});
// continue to next match page router
// if next page has shadow , fallthrough at serve-site
from_fallthrough = true;
continue;
}
}
} else {
response =
route.type === 'endpoint'
? await render_endpoint(event, await route.load())
: await render_page(event, route, options, state, resolve_opts);
if (is_page) {
if (from_fallthrough) {
const headers = new Headers({
'x-sveltekit-load': `${route_index}`
});
// next match not a shadow so fallthrough at client-side
return new Response(undefined, {
headers,
status: 204
});
}
response = await render_page(event, route, options, state, resolve_opts);
} else {
if (shadow_key || from_fallthrough) continue;
response = await render_endpoint(event, await route.load());
}
}

if (response) {
Expand Down Expand Up @@ -230,11 +251,22 @@ export async function respond(request, options, state = {}) {
});
}
}

return response;
}
}

// no match route
if (from_fallthrough) {
const headers = new Headers({
'x-sveltekit-load': '-1'
});
// next match not a shadow so fallthrough at client-side
return new Response(undefined, {
headers,
status: 204
});
}

// if this request came direct from the user, rather than
// via a `fetch` in a `load`, render a 404 page
if (!state.initiator) {
Expand All @@ -261,7 +293,6 @@ export async function respond(request, options, state = {}) {
throw new Error('request in handle has been replaced with event' + details);
}
});

// TODO for 1.0, change the error message to point to docs rather than PR
if (response && !(response instanceof Response)) {
throw new Error('handle must return a Response object' + details);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/** @type {import('@sveltejs/kit').RequestHandler} */
export async function get({ params }) {
const param = params.a;
if (param !== 'a') {
return {
fallthrough: true
};
}

return {
status: 200,
body: { param }
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script>
/** @type string|undefined */
export let param;
</script>
<h2>a-{param}</h2>
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/** @type {import('@sveltejs/kit').RequestHandler} */
export async function get({ params }) {
const param = params.b;
if (param !== 'b') {
return {
fallthrough: true
};
}

return {
status: 200,
body: { param }
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script>
/** @type string|undefined */
export let param;
</script>
<h2>b-{param}</h2>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<h2>c</h2>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<a href="/shadowed/fallthrough/a">fallthrough to shadow a</a>
<a id="b" href="/shadowed/fallthrough/b">fallthrough to shadow b</a>
<a href="/shadowed/fallthrough/c">fallthrough to no shadow c</a>
12 changes: 12 additions & 0 deletions packages/kit/test/apps/basics/test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,18 @@ test.describe.parallel('Shadowed pages', () => {
await clicknav('[href="/shadowed/dynamic/bar"]');
expect(await page.textContent('h1')).toBe('slug: bar');
});

test('Shadow fallthrough shadow', async ({ page, clicknav }) => {
await page.goto('/shadowed/fallthrough');
await clicknav('[href="/shadowed/fallthrough/b"]');
expect(await page.textContent('h2')).toBe('b-b');
});

test('Shadow fallthrough to no_shadow', async ({ page, clicknav }) => {
await page.goto('/shadowed/fallthrough');
await clicknav('[href="/shadowed/fallthrough/c"]');
expect(await page.textContent('h2')).toBe('c');
});
});

test.describe.parallel('Endpoints', () => {
Expand Down
3 changes: 2 additions & 1 deletion packages/kit/types/internal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export type CSRComponent = any; // TODO

export type CSRComponentLoader = () => Promise<CSRComponent>;

export type CSRRoute = [RegExp, CSRComponentLoader[], CSRComponentLoader[], GetParams?, HasShadow?];
export type CSRRoute = [RegExp, CSRComponentLoader[], CSRComponentLoader[], GetParams?, string?];

export interface EndpointData {
type: 'endpoint';
Expand Down Expand Up @@ -267,6 +267,7 @@ export interface SSROptions {
}

export interface SSRPage {
key: string;
type: 'page';
pattern: RegExp;
params: GetParams;
Expand Down