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

feat: Validate Vercel cron paths #9145

Merged
merged 15 commits into from
Feb 28, 2023
Merged

feat: Validate Vercel cron paths #9145

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7ce553c
feat: Validate Vercel cron paths
elliott-with-the-longest-name-on-github Feb 21, 2023
4d0b4fd
fix: Don't make Rich mad with bad variable names, changeset
elliott-with-the-longest-name-on-github Feb 21, 2023
6aa3e29
Update .changeset/tiny-deers-complain.md
Rich-Harris Feb 21, 2023
69ea8dc
fix: Better warning message + semantics
elliott-with-the-longest-name-on-github Feb 21, 2023
d712205
feat: Add type differentiator for endpoints / pages
elliott-with-the-longest-name-on-github Feb 21, 2023
6f8f40a
Merge branch 'elliott/vercel-cron-validation' of github.com:sveltejs/…
elliott-with-the-longest-name-on-github Feb 21, 2023
633e49f
fix: Better differentiators
elliott-with-the-longest-name-on-github Feb 21, 2023
3851208
feat: Add endpoint and page methods to adapter contract
elliott-with-the-longest-name-on-github Feb 23, 2023
07b650f
create ServerMetadataRoute interface
Rich-Harris Feb 26, 2023
764e948
simplify
Rich-Harris Feb 26, 2023
798ac47
simplify
Rich-Harris Feb 26, 2023
15bf138
move validation into separate function
Rich-Harris Feb 26, 2023
1a83086
flesh out error message
Rich-Harris Feb 26, 2023
206495b
endpoint -> api
Rich-Harris Feb 27, 2023
b26c604
endpoint -> api
Rich-Harris Feb 27, 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/tiny-deers-complain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/adapter-vercel': minor
---

feat: validate that Vercel cron paths match an API path
34 changes: 34 additions & 0 deletions packages/adapter-vercel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,40 @@ const plugin = function (defaults = {}) {
builder.rimraf(dir);
builder.rimraf(tmp);

if (fs.existsSync('./vercel.json')) {
const vercel_file = fs.readFileSync('./vercel.json', 'utf-8');
const vercel_config = JSON.parse(vercel_file);
const crons = /** @type {Array<unknown>} */ (
Array.isArray(vercel_config?.crons) ? vercel_config.crons : []
);
const GET_routes = builder.routes.filter(
(route) => route.type === 'endpoint' && route.methods.includes('GET')
);
/** @type {Array<string>} */
const unmatched_paths = [];

for (const cron of crons) {
if (typeof cron !== 'object' || cron === null || !('path' in cron)) {
continue;
}

const { path } = cron;
if (typeof path !== 'string') {
continue;
}

if (!GET_routes.some((route) => route.pattern.test(path))) {
unmatched_paths.push(path);
}
}

builder.log.warn(
`The following paths are not matched by any route:\n - ${unmatched_paths.join(
'\n - '
)}\nIf these paths are handled in your \`handle\` hook, you can safely ignore this warning.`
);
}

const files = fileURLToPath(new URL('./files', import.meta.url).href);

const dirs = {
Expand Down
1 change: 1 addition & 0 deletions packages/kit/src/core/adapt/builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export function create_builder({
/** @type {import('types').RouteDefinition} */
const facade = {
id: route.id,
type: route.endpoint ? 'endpoint' : 'page',
Copy link
Member

Choose a reason for hiding this comment

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

it needs to be a pair of booleans, not an enum — a route can have both +page and +server (in which case we do content negotiation)

Copy link
Member

Choose a reason for hiding this comment

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

Design wise I think this should be an enum with three values. Two booleans is incorrect in the sense that both can never be false at the same time

Copy link
Member

Choose a reason for hiding this comment

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

It's not future-proof — if we added a third route type (socket or something?), both would no longer make sense. Booleans are a better reflection of the types we use internally (where we went down this road before and switched to booleans because of the content negotiation thing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried again with booleans. Should the condition for hasPage be as simple as it is?

Also, is there a way around not false-positive-ing on a route with a page and an endpoint with a method other than GET?

Copy link
Member

@Rich-Harris Rich-Harris Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

good point re false positives. I guess we would need to do something like

{
  pageMethods: ['GET', 'POST'],
  endpointMethods: ['DELETE']
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that solution. Let me investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, is it possible for a page to have anything but GET?

Copy link
Member

Choose a reason for hiding this comment

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

yes, if it has form actions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented and pushed. I went with a slightly different object structure:

{
  endpoint: { methods: [] },
  page: { methods: [] }
}

Happy tochange it to the keys you suggested -- this just seemed more likely to grow without warts in the future.

segments: get_route_segments(route.id).map((segment) => ({
dynamic: segment.includes('['),
rest: segment.includes('[...'),
Expand Down
1 change: 1 addition & 0 deletions packages/kit/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,7 @@ export interface ResolveOptions {

export interface RouteDefinition<Config = any> {
id: string;
type: 'endpoint' | 'page';
pattern: RegExp;
prerender: PrerenderOption;
segments: RouteSegment[];
Expand Down