feat: add catch `all` handler
closes https://github.com/sveltejs/kit/issues/9164
Adds a catch-all handler that handles all unimplemented valid server requests. This is done by exporting the all function from a +server.js file.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
- [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
- [x] This message body should clearly illustrate what problems it solves.
- [x] Ideally, include a test that fails without this PR but passes with it.
Tests
- [x] Run the tests with
pnpm testand lint the project withpnpm lintandpnpm check
Changesets
- [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.
🦋 Changeset detected
Latest commit: 0ac4a791a8442fffd1b702b4ade4ccb61d417538
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| @sveltejs/kit | Minor |
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR
Have we thought about the effects this might have on https://kit.svelte.dev/docs/routing#server-content-negotiation ? I haven't thought about that all the way through yet. Also, if we decided (as in #9780) to start adding Vary: Accept to responses for requests with sibling GET/POST endpoints and pages, would this make that more difficult?
Have we thought about the effects this might have on kit.svelte.dev/docs/routing#server-content-negotiation ?
Content negotiation is still in play here, so the accept: text/html always connects the page.
Also, if we decided (as in #9780) to start adding
Vary: Acceptto responses for requests with siblingGET/POSTendpoints and pages, would this make that more difficult?
I'm not sure myself. Will need to start reading the MDN docs on what Vary: Accept is.
Also, if we decided (as in #9780) to start adding
Vary: Acceptto responses for requests with siblingGET/POSTendpoints and pages, would this make that more difficult?I'm not sure myself. Will need to start reading the MDN docs on what
Vary: Acceptis.
After reading MDN, it seems we would only need to return Vary: Accept, right? If so, it shouldn't impact this PR.
The server uses the Vary header to indicate which headers it actually used for content negotiation (or more precisely, the associated request headers), so that caches can work optimally.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation#server-driven_content_negotiation
@s3812497 it looks like this PR will need a rebase
reverted my previous merge with master and did it with rebase instead.
@s3812497 fixing the types revealed a type error here - can you look at it today? Else I'll look into it later.
FYI @s3812497 this now has a merge conflict since the HEAD PR was merged - if you could give this one more look that would be great 😄
Oh whoops, I completely missed that. I'll look into it now.
No worries, there was no chance to miss this on your side, I added this comment immediately after merging that other PR 😄
I tested this using MOVE and it came through. It works because of the way the if-else chain resolves. @Conduitry / @benmccann are you ok with that? I would be in favor of it, as it shuts down the "can you please add yet another export to +server.js" discussion once and for all.
If a route has a GET handler and an all handler, and a HEAD request comes in, it's not at all clear to me whether this should be handled with GET (implicitly discarding the response body, like we do now) or all. I could see both being expected behaviors by different people, and both with reasonable justifications.
So I don't know what to do about this. This makes me think that we're implementing the wrong feature, but I don't yet know what the right feature would be.
At one point I think we talked about having handle in subdirectories. If we had that, then I'm not sure if we'd still add this?
The HTTP HEAD method requests the headers that would be returned if the HEAD request's URL was instead requested with the HTTP GET method.
source: https://developer.mozilla.org/en-US/docs/web/http/methods/head
This makes it pretty clear that our automatic HEAD request handling must follow the path a GET request would take. So a defined GET function would take precedence over ALL.
Taking a step back I am still confused by the nature of this feature. Clearly the different request methods require different handling, so you'd always end up with a switch/case like statement inside ALL. So i don't see much convenience in this for known request methods.
This leaves support for unknown request methods, which could also be done by just checking for an all uppercase function of the method name?
The linked issue gives sufficient motivation IMO, it's somewhat niche but sometimes it's easiest to just forward everything to one function (and some libraries might be setup as such that they export one function which handles all different method types, which you then could just wire up). Since it solves more use cases than the "allow any uppercase export" solution I would prefer all.
I now agree with your reasoning of still prefering GET over all if it's present, we should adjust the logic in this PR accordingly.
I've updated the PR for GET to take precedence over all for HEAD requests.
I may have missed it - but is there a reason it's all not ALL? I feel like keeping it the same as the exported HTTP verbs (despite it not being one) would be less confusing
the reason is that ALL is not a valid http verb (even under the advanced ones) so it could be confusing to see that and then ask yourself "what kind of verb is that". all makes it visibly more distinct to know "ok this is something else".
all is still misleading or ambiguous. If you have POST and all, which one is handling an incoming post request? Isn't clear from the word all, which implies all requests are handled by it.
all is still misleading or ambiguous. If you have POST and all, which one is handling an incoming post request? Isn't clear from the word all, which implies all requests are handled by it.
I think it's probably fine, esp since the way people would learn about it is via docs which can explain it - however there could still be better words out there for it?
Other name suggestions were:
fallbackother- using the
defaultexport
Name suggestion: any - matches any request method not already exported.
I like fallback more for your description.
Why not just support this?
/** @type {import('@sveltejs/kit').[Handle](https://kit.svelte.dev/docs/types#public-types-handle)} */
export async function handle({ event, resolve }) {
if (event.url.pathname.startsWith('/custom')) {
return new Response('custom response');
}
const response = await resolve(event);
return response;
}
Why not just support this?
/** @type {import('@sveltejs/kit').[Handle](https://kit.svelte.dev/docs/types#public-types-handle)} */ export async function handle({ event, resolve }) { if (event.url.pathname.startsWith('/custom')) { return new Response('custom response'); } const response = await resolve(event); return response; }
Handling this case in the handle hook is already supported but would be made easier by this PR (directly exporting the fallback in the server endpoint and not cluttering the handle hook).
The export all handler has been renamed to fallback.
Why not just support this?
/** @type {import('@sveltejs/kit').[Handle](https://kit.svelte.dev/docs/types#public-types-handle)} */ export async function handle({ event, resolve }) { if (event.url.pathname.startsWith('/custom')) { return new Response('custom response'); } const response = await resolve(event); return response; }Handling this case in the
handlehook is already supported but would be made easier by this PR (directly exporting the fallback in the server endpoint and not cluttering the handle hook).
Right, no my suggestion was to allow +server.js to export a handler hook that is only hit within that route scope. It can be used like an all OR a fallback OR an after etc. without any ambiguity compared to the other exports in that module.
Another benefit would be scenarios where you have a common resolution you want to do once up front for a whole routing scope, almost like what you get with layouts in view routes.
Right, no my suggestion was to allow
+server.jsto export ahandlerhook that is only hit within that route scope. It can be used like anallOR afallbackOR anafteretc. without any ambiguity compared to the other exports in that module.
Sorry, I misunderstood. I'd definitely be in favour of subdirectory handle hooks replacing this as also mentioned by Ben
Right, no my suggestion was to allow
+server.jsto export ahandlerhook that is only hit within that route scope. It can be used like anallOR afallbackOR anafteretc. without any ambiguity compared to the other exports in that module.Sorry, I misunderstood. I'd definitely be in favour of subdirectory handle hooks replacing this as also mentioned by Ben
Ah I missed that earlier comment. I also don't see any replies to it. Is there some other location that subdirectory hooks has/is being discussed?
Honestly after thinking about it a bit I think there's certainly room for both a fallback and subdirectory hooks. 👍 this PR.