-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Throw when prerendered routes have unresolvable content negotiation #9994
Changes from 18 commits
cfcc741
d0d26c3
57a21e4
97f8ec9
9c66098
34e0ef4
8213ef6
df2c721
13c235d
118676a
4d16b8c
466feec
eb80b81
a90638e
1207d45
e384cce
5190bb3
da4b0bd
9999c99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@sveltejs/kit': patch | ||
--- | ||
|
||
fix: precedence of `entries` should be `+page => +page.server => +server` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@sveltejs/kit': patch | ||
--- | ||
|
||
fix: prerendered routes will throw when exposing both a GET handler and a page, preventing impossible content negotiation |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -21,10 +21,11 @@ export default forked(import.meta.url, analyse); | |||||
/** | ||||||
* @param {{ | ||||||
* manifest_path: string; | ||||||
* manifest_data: import('types').ManifestData; | ||||||
* env: Record<string, string> | ||||||
* }} opts | ||||||
*/ | ||||||
async function analyse({ manifest_path, env }) { | ||||||
async function analyse({ manifest_path, manifest_data, env }) { | ||||||
/** @type {import('@sveltejs/kit').SSRManifest} */ | ||||||
const manifest = (await import(pathToFileURL(manifest_path).href)).manifest; | ||||||
|
||||||
|
@@ -66,6 +67,10 @@ async function analyse({ manifest_path, env }) { | |||||
|
||||||
// analyse routes | ||||||
for (const route of manifest._.routes) { | ||||||
const route_data = /** @type {import('types').RouteData} */ ( | ||||||
manifest_data.routes.find((r) => r.id === route.id) | ||||||
); | ||||||
|
||||||
/** @type {Array<'GET' | 'POST'>} */ | ||||||
const page_methods = []; | ||||||
|
||||||
|
@@ -84,9 +89,12 @@ async function analyse({ manifest_path, env }) { | |||||
if (mod.prerender !== undefined) { | ||||||
validate_server_exports(mod, route.id); | ||||||
|
||||||
if (mod.prerender && (mod.POST || mod.PATCH || mod.PUT || mod.DELETE)) { | ||||||
if ( | ||||||
mod.prerender && | ||||||
(mod.POST || mod.PATCH || mod.PUT || mod.DELETE || mod.HEAD || mod.OPTIONS) | ||||||
) { | ||||||
throw new Error( | ||||||
`Cannot prerender a +server file with POST, PATCH, PUT, or DELETE (${route.id})` | ||||||
`Cannot prerender ${route_data.endpoint?.file} as it exposes POST, PATCH, PUT, DELETE, HEAD, or OPTIONS handlers` | ||||||
); | ||||||
} | ||||||
|
||||||
|
@@ -130,10 +138,21 @@ async function analyse({ manifest_path, env }) { | |||||
validate_page_exports(page.universal, page.universal_id); | ||||||
} | ||||||
|
||||||
prerender = get_option(nodes, 'prerender') ?? false; | ||||||
prerender = get_option(nodes, 'prerender') ?? prerender ?? false; | ||||||
|
||||||
config = get_config(nodes); | ||||||
entries ??= get_option(nodes, 'entries'); | ||||||
entries = get_option(nodes, 'entries') ?? entries; | ||||||
} | ||||||
|
||||||
if (prerender && route.endpoint && route.page) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't account for the
Suggested change
(though as stated in the earlier comment this might need a slight tweak either way |
||||||
const page = /** @type {string} */ ( | ||||||
route_data.leaf?.component ?? route_data.leaf?.universal ?? route_data.leaf?.server | ||||||
); | ||||||
const endpoint = /** @type {string} */ (route_data.endpoint?.file); | ||||||
|
||||||
throw new Error( | ||||||
`Cannot prerender both ${page} and ${endpoint}. Disable prerendering for this route, or delete one of the files.` | ||||||
); | ||||||
} | ||||||
|
||||||
metadata.routes.set(route.id, { | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,20 @@ | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
"name": "prerender-impossible-content-negotiation", | ||||||||||||||||||||||||||
"private": true, | ||||||||||||||||||||||||||
"version": "0.0.1", | ||||||||||||||||||||||||||
"scripts": { | ||||||||||||||||||||||||||
"dev": "vite dev", | ||||||||||||||||||||||||||
"build": "vite build", | ||||||||||||||||||||||||||
"preview": "vite preview", | ||||||||||||||||||||||||||
"check": "svelte-kit sync && tsc && svelte-check" | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
"devDependencies": { | ||||||||||||||||||||||||||
"@sveltejs/adapter-auto": "workspace:^", | ||||||||||||||||||||||||||
"@sveltejs/kit": "workspace:^", | ||||||||||||||||||||||||||
"svelte": "^3.56.0", | ||||||||||||||||||||||||||
"svelte-check": "^3.0.2", | ||||||||||||||||||||||||||
"typescript": "^4.9.4", | ||||||||||||||||||||||||||
"vite": "^4.3.6" | ||||||||||||||||||||||||||
Comment on lines
+12
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should keep these in sync with the other test projects
Suggested change
|
||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
"type": "module" | ||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<meta charset="utf-8" /> | ||
<link rel="icon" href="%sveltekit.assets%/favicon.png" /> | ||
<meta name="viewport" content="width=device-width" /> | ||
%sveltekit.head% | ||
</head> | ||
<body> | ||
<div>%sveltekit.body%</div> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
<h1>Hello world</h1> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export const prerender = true; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export function GET() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import adapter from '../../../../../adapter-auto/index.js'; | ||
|
||
/** @type {import('@sveltejs/kit').Config} */ | ||
const config = { | ||
kit: { | ||
adapter: adapter() | ||
} | ||
}; | ||
|
||
export default config; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
{ | ||
"compilerOptions": { | ||
"allowJs": true, | ||
"checkJs": true, | ||
"noEmit": true, | ||
"module": "esnext", | ||
"moduleResolution": "node", | ||
"paths": { | ||
"@sveltejs/kit": ["../../../../types"], | ||
"$lib": ["./src/lib"], | ||
"$lib/*": ["./src/lib/*"] | ||
} | ||
}, | ||
"extends": "./.svelte-kit/tsconfig.json" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import * as path from 'node:path'; | ||
import { sveltekit } from '@sveltejs/kit/vite'; | ||
|
||
/** @type {import('vite').UserConfig} */ | ||
const config = { | ||
build: { | ||
minify: false | ||
}, | ||
|
||
clearScreen: false, | ||
|
||
logLevel: 'silent', | ||
|
||
plugins: [sveltekit()], | ||
|
||
server: { | ||
fs: { | ||
allow: [path.resolve('../../../../src')] | ||
} | ||
} | ||
}; | ||
|
||
export default config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this catches all cases. If the latter of
+page.svelte
hasprerender
set totrue
and+server.js
has set it tofalse
then the finalprerender
value will befalse
, resulting in a false negative. I think we need to save both values independently and then check at the end.