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

breaking: stricter route validation, more correct config resolution #11256

Merged
merged 4 commits into from
Dec 11, 2023
Merged

breaking: stricter route validation, more correct config resolution #11256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34cb987
fix get_config
Rich-Harris Dec 10, 2023
c8309cc
oops
Rich-Harris Dec 10, 2023
a523b10
fix, changeset
Rich-Harris Dec 10, 2023
fb8cc47
always validate server exports
Rich-Harris Dec 11, 2023
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/eight-pens-help.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': major
---

breaking: fail if route with +page and +server is marked prerenderable
5 changes: 5 additions & 0 deletions .changeset/loud-plants-move.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': major
---

breaking: fail if +page and +server have mismatched config
5 changes: 5 additions & 0 deletions .changeset/mean-trees-notice.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: resolve route config correctly
162 changes: 90 additions & 72 deletions packages/kit/src/core/postbuild/analyse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,97 +52,54 @@ async function analyse({ manifest_path, env }) {
routes: new Map()
};

// analyse nodes
for (const loader of manifest._.nodes) {
const node = await loader();
const nodes = await Promise.all(manifest._.nodes.map((loader) => loader()));

// analyse nodes
for (const node of nodes) {
metadata.nodes[node.index] = {
has_server_load: node.server?.load !== undefined || node.server?.trailingSlash !== undefined
};
}

// analyse routes
for (const route of manifest._.routes) {
/** @type {Array<'GET' | 'POST'>} */
const page_methods = [];

/** @type {(import('types').HttpMethod | '*')[]} */
const api_methods = [];
const page =
route.page &&
analyse_page(
route.page.layouts.map((n) => (n === undefined ? n : nodes[n])),
nodes[route.page.leaf]
);

/** @type {import('types').PrerenderOption | undefined} */
let prerender = undefined;
/** @type {any} */
let config = undefined;
/** @type {import('types').PrerenderEntryGenerator | undefined} */
let entries = undefined;
const endpoint = route.endpoint && analyse_endpoint(route, await route.endpoint());

if (route.endpoint) {
const mod = await route.endpoint();
if (mod.prerender !== undefined) {
validate_server_exports(mod, route.id);
if (page?.prerender && endpoint?.prerender) {
throw new Error(`Cannot prerender a route with both +page and +server files (${route.id})`);
}

if (mod.prerender && (mod.POST || mod.PATCH || mod.PUT || mod.DELETE)) {
if (page?.config && endpoint?.config) {
for (const key in { ...page.config, ...endpoint.config }) {
if (JSON.stringify(page.config[key]) !== JSON.stringify(endpoint.config[key])) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it guaranteed that the keys of both of these are in the same order? i.e. could it fail because content is the same but in different order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Below the top level, yes. I didn't think it was worth adding deep equality checking to handle that case, we're in super niche territory

throw new Error(
`Cannot prerender a +server file with POST, PATCH, PUT, or DELETE (${route.id})`
`Mismatched route config for ${route.id} — the +page and +server files must export the same config, if any`
);
}

prerender = mod.prerender;
}

Object.values(mod).forEach((/** @type {import('types').HttpMethod} */ method) => {
if (mod[method] && ENDPOINT_METHODS.has(method)) {
api_methods.push(method);
} else if (mod.fallback) {
api_methods.push('*');
}
});

config = mod.config;
entries = mod.entries;
}

if (route.page) {
const nodes = await Promise.all(
[...route.page.layouts, route.page.leaf].map((n) => {
if (n !== undefined) return manifest._.nodes[n]();
})
);

const layouts = nodes.slice(0, -1);
const page = nodes.at(-1);

for (const layout of layouts) {
if (layout) {
validate_layout_server_exports(layout.server, layout.server_id);
validate_layout_exports(layout.universal, layout.universal_id);
}
}

if (page) {
page_methods.push('GET');
if (page.server?.actions) page_methods.push('POST');

validate_page_server_exports(page.server, page.server_id);
validate_page_exports(page.universal, page.universal_id);
}

prerender = get_option(nodes, 'prerender') ?? false;

config = get_config(nodes);
entries ??= get_option(nodes, 'entries');
}
const page_methods = page?.methods ?? [];
const api_methods = endpoint?.methods ?? [];
const entries = page?.entries ?? endpoint?.entries;

metadata.routes.set(route.id, {
config,
config: page?.config ?? endpoint?.config,
methods: Array.from(new Set([...page_methods, ...api_methods])),
page: {
methods: page_methods
},
api: {
methods: api_methods
},
prerender,
prerender: page?.prerender ?? endpoint?.prerender,
entries:
entries && (await entries()).map((entry_object) => resolvePath(route.id, entry_object))
});
Expand All @@ -151,20 +108,81 @@ async function analyse({ manifest_path, env }) {
return metadata;
}

/**
* @param {import('types').SSRRoute} route
* @param {import('types').SSREndpoint} mod
*/
function analyse_endpoint(route, mod) {
validate_server_exports(mod, route.id);

if (mod.prerender && (mod.POST || mod.PATCH || mod.PUT || mod.DELETE)) {
throw new Error(
`Cannot prerender a +server file with POST, PATCH, PUT, or DELETE (${route.id})`
);
}

/** @type {Array<import('types').HttpMethod | '*'>} */
const methods = [];

Object.values(mod).forEach((/** @type {import('types').HttpMethod} */ method) => {
if (mod[method] && ENDPOINT_METHODS.has(method)) {
methods.push(method);
} else if (mod.fallback) {
methods.push('*');
}
});

return {
config: mod.config,
entries: mod.entries,
methods,
prerender: mod.prerender ?? false
};
}

/**
* @param {Array<import('types').SSRNode | undefined>} layouts
* @param {import('types').SSRNode} leaf
*/
function analyse_page(layouts, leaf) {
for (const layout of layouts) {
if (layout) {
validate_layout_server_exports(layout.server, layout.server_id);
validate_layout_exports(layout.universal, layout.universal_id);
}
}

/** @type {Array<'GET' | 'POST'>} */
const methods = ['GET'];
if (leaf.server?.actions) methods.push('POST');

validate_page_server_exports(leaf.server, leaf.server_id);
validate_page_exports(leaf.universal, leaf.universal_id);

return {
config: get_config([...layouts, leaf]),
entries: leaf.universal?.entries ?? leaf.server?.entries,
methods,
prerender: get_option([...layouts, leaf], 'prerender') ?? false
};
}

/**
* Do a shallow merge (first level) of the config object
* @param {Array<import('types').SSRNode | undefined>} nodes
*/
function get_config(nodes) {
/** @type {any} */
let current = {};

for (const node of nodes) {
const config = node?.universal?.config ?? node?.server?.config;
if (config) {
current = {
...current,
...config
};
}
if (!node?.universal?.config && !node?.server?.config) continue;

current = {
...current,
...node?.universal?.config,
...node?.server?.config
};
}

return Object.keys(current).length ? current : undefined;
Expand Down
Loading