-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Method overrides #1046
Comments
Looking at the readme of that middleware, are they actually blessing that particular query parameter name? The only thing I saw blessed was a particular HTTP header. Or are you saying this would be off by default and people would need to explicitly opt in and say what query parameter they want to use? |
They're blessing it in the sense that it's in the README. I've seen it used in a couple of different places I'm suggesting that // svelte.config.cjs
module.exports = {
kit: {
methodOverride: {
key: 'httpverb',
allowed: ['patch']
}
}
}; |
I think not having this be opt-in would just bother me, no matter what name we used or what level of blessed this was in that middleware. It could be that I just wasn't looking, but I've certainly not gotten the impression that this has been at all an established de facto standard. |
I don't know if I'd go as far as saying it's a de facto standard, just that it's a cowpath. Can you articulate why it bothers you? The framework is already doing other frameworky things like generating 304s and parsing bodies and serving static assets, is this so different? |
That a 304 means a redirect and that a given If this is an appeal to "2.4 million downloads of this package per month can't be wrong" then I will remind you about other commonly done things that you have complained about vehemently in the past. And if you still think this is a good idea, I'm not going to keep arguing about it, because it doesn't seem that awful as a default. |
The only reason I'm keen on it is that without method overrides there's basically no way to build a CRUD app that doesn't rely on JS, AFAICT, and if 'you can build robust apps that don't make anti-SPA-fanatics froth at the mouth' is one of the selling points of SvelteKit (which I think it is!) then maybe it warrants being part of the framework. At the same time, I've rarely regretted listening to you in the past. I think we need some more opinions! |
For consideration - prior art using the |
Thanks, that's very useful context. I see a few of these: <input type="hidden" name="_method" value="PUT"> Perhaps we should support both? In which case it probably would need to be in the framework, since that would be more finicky to do in userland |
I'm in favor of this proposal. It's something I don't want to have to think about implementing as a user and would be happy to have the framework handle. The only danger I see is if I had a form with an input named |
I'm not at all against this being included in the framework; what I'm concerned about is this being non-standard behavior that is being sent to folks as a default. |
Gross 😅 Better to scaffold a server-side Methods and status codes are fundamental building blocks. Paths can be reworked if need be. And there's a much deeper cow path for shuttling everything thru POSTs if needed. |
Huh. I find it much grosser to update or delete a resource with a POST request. If HTTP methods mean anything at all, then |
I like this option supported in svelte kit. In Laravel Framework - method spoofing as mention by @GrygrFlzr, when do something about update or delete data like
OR
|
I'd be interested in tackling this. I think there would be value in providing both of these approaches for overriding: <form method="post" action="/todos/{id}?_method=put"> AND <input type="hidden" name="_method" value="PUT"> The question of whether or not it should be enabled by default is interesting. Enabling by default could create some unexpected and confusing behavior for anyone unaware of the functionality. But I have to imagine the odds of someone submitting a form with a I can start working on a solution based on the discussion here and we can go from there. |
An attacker would increase the odds of that parameter quite a bit ;). This is related to #72, because right now if someone is using Also the name It would probably be best to look at some existing implementations, assuming they already figured out in all these things. https://github.com/expressjs/method-override/blob/5b83d4f0dc3db414df6c7e4a5da93dec170153de/index.js#L52-L55 Edit: if anyone wants to play with this const express = require('express');
const methodOverride = require('method-override')
const app = express();
app.use(methodOverride('_method'))
app.use((req, res, next) => {
console.log(req.method);
next();
});
app.listen(1337);
This will log |
Also don't just take my word for it, these things are happening right now
|
So from your points, here's a list of minimum requirements for this functionality (some of these were planned already, given discussions above and mirroring other solutions):
|
TIL that polka supports more methods than I expected (https://github.com/lukeed/trouter/blob/0692417c728a4f6aecbc350f714019baf3072bc7/index.mjs#L7-L16) and express actually exposes every single one that the HTTP parser supports (https://github.com/expressjs/express/blob/06d11755c99fe4c1cddf8b889a687448b568472d/lib/router/index.js#L506-L513). But for SvelteKit I think it's safe to limit them to these three until someone complains? What methods do SvelteKit endpoints support? I somewhat doubt Cloudflare Workers, Netlify or Vercel support all of them and that their reverse proxies let all of them through. But maybe I'm just too paranoid. But I'd be really curious if someone actually has a use-case for TRACE or CONNECT inside SvelteKit, that would not be better off in a custom entryPoint in the node adapter. Same for all the WebDAV methods.
I don't think that's a problem, DELETE is not inherently worse than any other. Overwriting things via PUT has a similar impact as DELETE. Or PUTing your crypto address to override the victims address to receive their funds is arguable worse than just deleting theirs. |
FWIW, a user's perspective: Just stumbled over this First, I wanted to adapt your solution but I decided just to use POST for any mutation and GET for all queries, and nothing else. I use on the storage-side anyway kind of a super-powered upsert for all mutations. This can create new objects, replace existing ones and/or patch existing ones (all paired with strict types if anyone cares, so it's not a mess, haha). None of the HTTP verbs matches this upsert behavior exactly (it's a blend of PUT and PATCH), so I thought, why all this fuzz, mental drain for nothing. Besides, I find almost all http verbs unintuitive, not reflecting what they do, except GET and DELETE. Not sure how I gonna deal with DELETEs yet, maybe just POST an empty object or an extra flag, IDK yet, latter ideas feel also hacky, so I might opt for the method override just for DELETEs. However and in general, if this method override is done in prior frameworks and learned by the users, why not. |
* Add functionality to override http methods (issue #1046) See: #1046 * Removing changes to lockfile * Validate allowed_methods does not contain GET, rephrase docs * Remove hidden field strategy and enabled config, rename other config values * lint/format * tweak error messages * slim docs down * tweak changeset Co-authored-by: Rich Harris <[email protected]>
Resolved by #2989 |
I could have sworn we'd already talked about this at some point, but I guess not.
Is your feature request related to a problem? Please describe.
Bewilderingly, the only valid
<form>
methods are GET and POST — in other words<form method="put">
is invalid, and will result inform.method === 'get'
.A popular workaround is to use a method override query string parameter, like so:
(As evidence for the validity of this approach, there's an official Express middleware for it: https://www.npmjs.com/package/method-override).
It would be nice if such a thing existed in SvelteKit as well, since we want to encourage people to build apps that work with JavaScript disabled, and that's only really possible if you can use PUT and DELETE alongside GET and POST without having to resort to
fetch
.Describe the solution you'd like
Since the
?_method=put
cowpath already exists, I propose that we bake it into SvelteKit, while making it configurable:Then, when any request that comes in where
query.has(override)
, we change the request method before passing it tohandle
.How important is this feature to you?
It would be possible to do in userland...
...but I think this is the sort of thing that belongs in the framework.
The text was updated successfully, but these errors were encountered: