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 3 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('1', `'${route.key}'`);
aolose marked this conversation as resolved.
Show resolved Hide resolved
return `// ${route.a[route.a.length - 1]}\n\t\t[${tuple.join(', ')}]`;
}
})
Expand Down
34 changes: 19 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,26 @@ 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[]}**/
aolose marked this conversation as resolved.
Show resolved Hide resolved
const ends = [];
aolose marked this conversation as resolved.
Show resolved Hide resolved
routes.forEach((route, i) => {
const { key, type } = route;
if (type === 'page') {
pages.set(key, route);
aolose marked this conversation as resolved.
Show resolved Hide resolved
} else {
ends.unshift(i);
aolose marked this conversation as resolved.
Show resolved Hide resolved
}
}
});
ends.forEach((i) => {
aolose marked this conversation as resolved.
Show resolved Hide resolved
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
4 changes: 3 additions & 1 deletion packages/kit/src/core/dev/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ export async function create_plugin(config, cwd) {
}),
routes: manifest_data.routes.map((route) => {
if (route.type === 'page') {
/**@type import('types').SSRPage**/
aolose marked this conversation as resolved.
Show resolved Hide resolved
return {
key: route.key,
type: 'page',
pattern: route.pattern,
params: get_params(route.params),
Expand All @@ -119,7 +121,7 @@ export async function create_plugin(config, cwd) {
b: route.b.map((id) => manifest_data.components.indexOf(id))
};
}

/**@type import('types').SSREndpoint**/
aolose marked this conversation as resolved.
Show resolved Hide resolved
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
8 changes: 4 additions & 4 deletions packages/kit/src/runtime/client/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ export class Renderer {
if (cached) return cached;
}

const [pattern, a, b, get_params, has_shadow] = route;
const [pattern, a, b, get_params, has_shadow, routeKey] = route;
aolose marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -775,12 +775,12 @@ export class Renderer {
const res = await fetch(
`${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`,
{
// @ts-ignore
headers: {
'x-sveltekit-load': 'true'
'x-sveltekit-load': routeKey
aolose marked this conversation as resolved.
Show resolved Hide resolved
}
}
);

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

Expand All @@ -791,7 +791,7 @@ export class Renderer {
state: this.current
};
}

if (res.status === 204) return;
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 this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a request endpoint from shadow page and endpoints return a fallthrough,
then client-side need to know the response is a fallthrough.
We need a flag to tell the client-side that. so I return a 204 status to client-site.

props = await res.json();
} else {
status = res.status;
Expand Down
41 changes: 34 additions & 7 deletions packages/kit/src/runtime/server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,38 @@ export async function respond(request, options, state = {}) {
event.url = new URL(event.url.origin + normalized + event.url.search);
}

const requestRouteKey = request.headers.get('x-sveltekit-load');
if (requestRouteKey !== null) {
const route = options.manifest._.routes.find(
(route) => route.type === 'page' && route.key === requestRouteKey
);
if (route) {
// @ts-ignore
const shadow = route.shadow;
if (shadow) {
const match = route.pattern.exec(decoded);
if (match) {
event.params = route.params ? decode_params(route.params(match)) : {};
const response = await render_endpoint(event, await shadow());
if (response) {
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);
return new Response(undefined, {
status: 204,
headers
});
}
}
return response;
}
}
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Could this not be replaced with something along these lines inside the for loop below?

if (key && route.key !== key) continue;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's better.

for (const route of options.manifest._.routes) {
const match = route.pattern.exec(decoded);
if (!match) continue;
Expand All @@ -164,7 +196,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 (request.headers.get('x-sveltekit-load') === 'true') {
if (requestRouteKey !== null) {
if (response) {
// since redirects are opaque to the browser, we need to repackage
// 3xx responses as 200s with a custom header
Expand All @@ -183,11 +215,7 @@ export async function respond(request, options, state = {}) {
} 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'
}
});
return new Response(null, { status: 204 });
}
}
} else {
Expand Down Expand Up @@ -261,7 +289,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
17 changes: 16 additions & 1 deletion packages/kit/types/internal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,14 @@ 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?,
HasShadow?,
aolose marked this conversation as resolved.
Show resolved Hide resolved
string?
];

export interface EndpointData {
type: 'endpoint';
Expand Down Expand Up @@ -211,6 +218,7 @@ export interface SSREndpoint {
type: 'endpoint';
pattern: RegExp;
params: GetParams;

aolose marked this conversation as resolved.
Show resolved Hide resolved
load(): Promise<{
[method: string]: RequestHandler;
}>;
Expand All @@ -236,7 +244,9 @@ export interface SSROptions {
dev: boolean;
floc: boolean;
get_stack: (error: Error) => string | undefined;

aolose marked this conversation as resolved.
Show resolved Hide resolved
handle_error(error: Error & { frame?: string }, event: RequestEvent): void;

aolose marked this conversation as resolved.
Show resolved Hide resolved
hooks: Hooks;
hydrate: boolean;
manifest: SSRManifest;
Expand All @@ -247,10 +257,13 @@ export interface SSROptions {
};
prefix: string;
prerender: boolean;

aolose marked this conversation as resolved.
Show resolved Hide resolved
read(file: string): Buffer;

aolose marked this conversation as resolved.
Show resolved Hide resolved
root: SSRComponent['default'];
router: boolean;
service_worker?: string;

aolose marked this conversation as resolved.
Show resolved Hide resolved
template({
head,
body,
Expand All @@ -262,11 +275,13 @@ export interface SSROptions {
assets: string;
nonce: string;
}): string;

aolose marked this conversation as resolved.
Show resolved Hide resolved
template_contains_nonce: boolean;
trailing_slash: TrailingSlash;
}

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