Skip to content

fix: enhance and tighten up Vercel adapter warnings #9436

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

Merged
merged 2 commits into from
Mar 17, 2023
Merged
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
5 changes: 5 additions & 0 deletions .changeset/forty-suns-listen.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/adapter-vercel': minor
---

feat: warn when prerender setting makes isr config useless
5 changes: 5 additions & 0 deletions .changeset/polite-islands-glow.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/adapter-vercel': patch
---

fix: don't show cron warning when everything's valid
5 changes: 5 additions & 0 deletions .changeset/tasty-weeks-rush.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/adapter-vercel': patch
---

fix: allow to set isr to false to clear isr config in leafs
36 changes: 19 additions & 17 deletions packages/adapter-vercel/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,25 @@ export interface ServerlessConfig {
* [Incremental Static Regeneration](https://vercel.com/docs/concepts/incremental-static-regeneration/overview) configuration.
* Serverless only.
*/
isr?: {
/**
* Expiration time (in seconds) before the cached asset will be re-generated by invoking the Serverless Function. Setting the value to `false` means it will never expire.
*/
expiration: number | false;
/**
* Random token that can be provided in the URL to bypass the cached version of the asset, by requesting the asset
* with a __prerender_bypass=<token> cookie.
*
* Making a `GET` or `HEAD` request with `x-prerender-revalidate: <token>` will force the asset to be re-validated.
*/
bypassToken?: string;
/**
* List of query string parameter names that will be cached independently. If an empty array, query values are not considered for caching. If undefined each unique query value is cached independently
*/
allowQuery?: string[] | undefined;
};
isr?:
| {
/**
* Expiration time (in seconds) before the cached asset will be re-generated by invoking the Serverless Function. Setting the value to `false` means it will never expire.
*/
expiration: number | false;
/**
* Random token that can be provided in the URL to bypass the cached version of the asset, by requesting the asset
* with a __prerender_bypass=<token> cookie.
*
* Making a `GET` or `HEAD` request with `x-prerender-revalidate: <token>` will force the asset to be re-validated.
*/
bypassToken?: string;
/**
* List of query string parameter names that will be cached independently. If an empty array, query values are not considered for caching. If undefined each unique query value is cached independently
*/
allowQuery?: string[] | undefined;
}
| false;
}

export interface EdgeConfig {
Expand Down
58 changes: 45 additions & 13 deletions packages/adapter-vercel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,21 @@ const plugin = function (defaults = {}) {
/** @type {Map<import('@sveltejs/kit').RouteDefinition<import('.').Config>, { expiration: number | false, bypassToken: string | undefined, allowQuery: string[], group: number, passQuery: true }>} */
const isr_config = new Map();

/** @type {Set<string>} */
const ignored_isr = new Set();

// group routes by config
for (const route of builder.routes) {
if (route.prerender === true) continue;
const runtime = route.config?.runtime ?? defaults?.runtime ?? get_default_runtime();
const config = { runtime, ...defaults, ...route.config };

const pattern = route.pattern.toString();
if (is_prerendered(route)) {
if (config.isr) {
ignored_isr.add(route.id);
}
continue;
}

const runtime = route.config?.runtime ?? defaults?.runtime ?? get_default_runtime();
if (runtime && !VALID_RUNTIMES.includes(runtime)) {
throw new Error(
`Invalid runtime '${runtime}' for route ${
Expand All @@ -168,8 +176,6 @@ const plugin = function (defaults = {}) {
);
}

const config = { runtime, ...defaults, ...route.config };

if (config.isr) {
const directory = path.relative('.', builder.config.kit.files.routes + route.id);

Expand Down Expand Up @@ -197,6 +203,7 @@ const plugin = function (defaults = {}) {
const hash = hash_config(config);

// first, check there are no routes with incompatible configs that will be merged
const pattern = route.pattern.toString();
const existing = conflicts.get(pattern);
if (existing) {
if (existing.hash !== hash) {
Expand All @@ -219,6 +226,20 @@ const plugin = function (defaults = {}) {
group.routes.push(route);
}

if (ignored_isr.size) {
builder.log.warn(
`\nWarning: The following routes have an ISR config which is ignored because the route is prerendered:`
Copy link
Member Author

Choose a reason for hiding this comment

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

I made this a warning, thought about whether or not this is an error, but I think that would be breaking - maybe in the next major? Or would this be more annoying than helpful if it was an error?

Copy link
Member

Choose a reason for hiding this comment

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

I think a warning is probably fine, you could have a prerendered leaf within an ISR'd group, for example. we can see if people trip up on it in practice

);

for (const ignored of ignored_isr) {
console.log(` - ${ignored}`);
}

console.log(
'Either remove the "prerender" option from these routes to use ISR, or remove the ISR config.\n'
);
}

const singular = groups.size === 1;

for (const group of groups.values()) {
Expand All @@ -240,7 +261,7 @@ const plugin = function (defaults = {}) {
}

for (const route of builder.routes) {
if (route.prerender === true) continue;
if (is_prerendered(route)) continue;

const pattern = route.pattern.toString();

Expand Down Expand Up @@ -456,7 +477,7 @@ async function create_function_bundle(builder, entry, dir, config) {
if (resolution_failures.size > 0) {
const cwd = process.cwd();
builder.log.warn(
'The following modules failed to locate dependencies that may (or may not) be required for your app to work:'
'Warning: The following modules failed to locate dependencies that may (or may not) be required for your app to work:'
);

for (const [importer, modules] of resolution_failures) {
Expand Down Expand Up @@ -572,15 +593,26 @@ function validate_vercel_json(builder, vercel_config) {
unmatched_paths.push(path);
}

builder.log.warn(
`\nvercel.json defines cron tasks that use paths that do not correspond to an API route with a GET handler (ignore this if the request is handled in your \`handle\` hook):`
);
if (unmatched_paths.length) {
builder.log.warn(
`\nWarning: vercel.json defines cron tasks that use paths that do not correspond to an API route with a GET handler (ignore this if the request is handled in your \`handle\` hook):`
);

for (const path of unmatched_paths) {
console.log(` - ${path}`);
}

for (const path of unmatched_paths) {
console.log(` - ${path}`);
console.log('');
}
}

console.log('');
/** @param {import('@sveltejs/kit').RouteDefinition} route */
function is_prerendered(route) {
return (
route.prerender === true ||
(route.prerender === 'auto' &&
route.segments.every((segment) => !segment.dynamic))
);
}

export default plugin;
2 changes: 1 addition & 1 deletion packages/kit/types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export interface Builder {
config: ValidatedConfig;
/** Information about prerendered pages and assets, if any. */
prerendered: Prerendered;
/** An array of dynamic (not prerendered) routes */
/** An array of all routes (including prerendered) */
Copy link
Member Author

Choose a reason for hiding this comment

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

drive-by comment fix - I don't think we need a changeset for this, it's okay to just release it as part of the next Kit release without special mention

routes: RouteDefinition[];

/**
Expand Down