kit icon indicating copy to clipboard operation
kit copied to clipboard

feat: add catch `all` handler

Open teemingc opened this issue 2 years ago • 6 comments

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 test and lint the project with pnpm lint and pnpm 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 changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

teemingc avatar Apr 23 '23 17:04 teemingc

🦋 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

changeset-bot[bot] avatar Apr 23 '23 17:04 changeset-bot[bot]

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?

Conduitry avatar Apr 27 '23 11:04 Conduitry

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: Accept to responses for requests with sibling GET/POST endpoints 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.

teemingc avatar Apr 27 '23 12:04 teemingc

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?

I'm not sure myself. Will need to start reading the MDN docs on what Vary: Accept is.

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

teemingc avatar Apr 29 '23 11:04 teemingc

@s3812497 it looks like this PR will need a rebase

benmccann avatar May 27 '23 04:05 benmccann

reverted my previous merge with master and did it with rebase instead.

teemingc avatar May 27 '23 08:05 teemingc

@s3812497 fixing the types revealed a type error here - can you look at it today? Else I'll look into it later.

dummdidumm avatar Jul 04 '23 13:07 dummdidumm

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 😄

dummdidumm avatar Jul 04 '23 13:07 dummdidumm

Oh whoops, I completely missed that. I'll look into it now.

teemingc avatar Jul 04 '23 13:07 teemingc

No worries, there was no chance to miss this on your side, I added this comment immediately after merging that other PR 😄

dummdidumm avatar Jul 04 '23 13:07 dummdidumm

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.

dummdidumm avatar Jul 04 '23 15:07 dummdidumm

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.

Conduitry avatar Jul 04 '23 22:07 Conduitry

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?

benmccann avatar Jul 05 '23 00:07 benmccann

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?

dominikg avatar Jul 05 '23 08:07 dominikg

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.

dummdidumm avatar Jul 05 '23 10:07 dummdidumm

I've updated the PR for GET to take precedence over all for HEAD requests.

teemingc avatar Jul 05 '23 16:07 teemingc

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

ghostdevv avatar Jul 07 '23 02:07 ghostdevv

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".

dummdidumm avatar Jul 07 '23 07:07 dummdidumm

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.

dominikg avatar Jul 07 '23 07:07 dominikg

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?

ghostdevv avatar Jul 09 '23 23:07 ghostdevv

Other name suggestions were:

  • fallback
  • other
  • using the default export

karimfromjordan avatar Jul 10 '23 03:07 karimfromjordan

Name suggestion: any - matches any request method not already exported.

teemingc avatar Jul 10 '23 09:07 teemingc

I like fallback more for your description.

gtm-nayan avatar Jul 10 '23 10:07 gtm-nayan

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;
}

matthewrobb avatar Jul 20 '23 18:07 matthewrobb

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).

teemingc avatar Jul 21 '23 05:07 teemingc

The export all handler has been renamed to fallback.

teemingc avatar Jul 21 '23 05:07 teemingc

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).

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.

matthewrobb avatar Jul 21 '23 14:07 matthewrobb

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.

Sorry, I misunderstood. I'd definitely be in favour of subdirectory handle hooks replacing this as also mentioned by Ben

teemingc avatar Jul 23 '23 12:07 teemingc

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.

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?

matthewrobb avatar Jul 25 '23 20:07 matthewrobb

Honestly after thinking about it a bit I think there's certainly room for both a fallback and subdirectory hooks. 👍 this PR.

matthewrobb avatar Jul 26 '23 13:07 matthewrobb