feat: Validate Vercel cron paths#9145
Conversation
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
| /** @type {import('types').RouteDefinition} */ | ||
| const facade = { | ||
| id: route.id, | ||
| type: route.endpoint ? 'endpoint' : 'page', |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
good point re false positives. I guess we would need to do something like
{
pageMethods: ['GET', 'POST'],
endpointMethods: ['DELETE']
}
There was a problem hiding this comment.
I like that solution. Let me investigate.
There was a problem hiding this comment.
Wait, is it possible for a page to have anything but GET?
There was a problem hiding this comment.
yes, if it has form actions
There was a problem hiding this comment.
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.
|
Should it be considered to add |
That's what Next is doing but I think it's the wrong approach — if used with a route with dynamic params, then you need some mechanism for specifying It also means that you specify cron tasks in a SvelteKit project differently to how you do it for every other framework, which means more documentation and confusion. Much simpler to just use |
Yes it completely breaks down when you need dynamic params. |
|
If this makes use of a new adapter API it needs to be a major version bump for adapter-vercel, which I'd like to avoid. Can we code it such that the validation only happens if you have a recent enough SvelteKit version which contains these APIs? |
|
Is it necessary to trigger that warning? I feel it's kinda strange to only check for |
|
It's not strictly necessary, but it is helpful -- and without the warning it's a potentially confusing issue. And I'm not really sure what other |
|
@dummdidumm any ideas? The obvious one is: // Remove this check for v3
if (routes.endpoints !== undefined) {
// Code
} |
Rich-Harris
left a comment
There was a problem hiding this comment.
Have made it backwards-compatible.
Code looks fine now, but I'm uneasy about exposing endpoint in a public API. It's terminology that still appears in the docs in a few places but we had generally decided to stop using it in favour of 'API route' which is more common. I still think route.server would be the best option (route.server and route.page corresponds to +server and +page), but route.api could also work.
|
I'm more in favor of |
|
I vote for |
|
|
Vercel Cron Jobs are launching. Thanks to their API, we don't actually have to make any changes to support them. However, as a courtesy to our users, we can make sure that a SvelteKit app exports a
GETAPI handler that matches the path for each cron job. We can't do more than warn here, as users could handle these requests inhandle, but I think the normal usecase would be to define a+server.tsendpoint.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.