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.

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

PATCH verb #50

Closed
lukeapage opened this issue May 7, 2015 · 16 comments
Closed

PATCH verb #50

lukeapage opened this issue May 7, 2015 · 16 comments

Comments

@lukeapage
Copy link

@lukeapage lukeapage commented May 7, 2015

Should the patch verb be included in the spec? (https://tools.ietf.org/html/rfc5789)

here -
https://fetch.spec.whatwg.org/#concept-method-normalize

and
https://fetch.spec.whatwg.org/#http-network-or-cache-fetch

If request's body is null and request's method is `HEAD`, `POST`, or `PUT`, set contentLengthValue to `0`.
@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented May 7, 2015

No, PATCH will work as-is.

@annevk annevk closed this May 7, 2015
@opengeek

This comment has been minimized.

Copy link

@opengeek opengeek commented Feb 24, 2016

I think there is confusion here. PATCH is not explicitly listed in the HTTP verbs for normalization, which is the main problem. This means it is not normalized to uppercase like the rest of the HTTP verbs, and thus if a library attempts to use patch (lowercase), everything comes to a screeching halt, since the lowercase patch is not in the allowed methods for CORS. If the documentation can be modified to explicitly include PATCH in the list of HTTP verbs to normalize, I think this issue can be resolved easily.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Feb 24, 2016

HTTP verbs are case-sensitive. That we normalize a couple of them is already, strictly speaking, against the rules, but we have to do so for compatibility. If you want to use PATCH, write PATCH. If you want patch, write patch. If you want Patch, write Patch. Etc. None of this has anything to do with CORS, they should all work equally well.

@opengeek

This comment has been minimized.

Copy link

@opengeek opengeek commented Feb 24, 2016

This is ridiculous for consistency when writing code that interacts with fetch. Yeah, we normalize all HTTP methods, except PATCH BTW. I'm not following your justification, at all.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Feb 24, 2016

Why? Just write the verb in the correct case and there's no issue. (And we're not even close to normalizing all known HTTP methods, let alone all possible HTTP methods.)

@opengeek

This comment has been minimized.

Copy link

@opengeek opengeek commented Feb 24, 2016

Because every library that exists that adopts fetch or might adopt fetch in the future, already expects the normalization of HTTP verbs. When they make the switch to fetch, everything appears fine, until they realize that only PATCH is not normalized downstream. This is the only typical RESTful method that is NOT normalized and the inconsistency causes hard-to-track-down bugs in production applications when they update dependencies that suddenly start using fetch instead of some previous library that normalized all known HTTP verbs.

It's about consistency and common usage.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Feb 24, 2016

Do you have some data to back that up? I would expect such libraries to adhere to HTTP and treat verbs as case-sensitive. Why would any library that deals with HTTP expect these to be byte-uppercased? That flat out contradicts HTTP.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Feb 24, 2016

Let me try this another way, I tend to agree that byte-uppercasing every verb/method would be predictable and easier, but the HTTP folks would be quite upset since we have just ruled out a ton of legitimate methods. That's why we only do it for the subset browsers were already doing this for.

@opengeek

This comment has been minimized.

Copy link

@opengeek opengeek commented Feb 24, 2016

See marmelab/restful.js#81 and the related issue at github/fetch#37 — we just found the bug in a production application yesterday after missing this problem when updating to the restful.js library version refactored on top of fetch. I think you can see where the confusion is carrying down from the spec.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Feb 24, 2016

What did you rely on before? You had some abstraction that uppercased everything? Or did you rely on a bug in certain browsers where PATCH is sometimes byte-uppercased in XMLHttpRequest?

@opengeek

This comment has been minimized.

Copy link

@opengeek opengeek commented Feb 24, 2016

To be honest, i didn't follow that rabbit hole all the way down. I'm just a consumer of the restful.js library and it seemed that the bug was coming from some confusion over whether fetch should normalize the PATCH method or not, based on the spec. Seems like the authors of restful.js simply expected it to be normalized inside fetch just like the rest of the methods they were using. I'm not saying this is correct, but this seems like a problem that could easily be avoided in other projects that might be expecting this same behavior.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Feb 24, 2016

As I mentioned, we'd break projects that are using HTTP correctly and want to use patcH for some reason. Unless we switch to always byte-uppercasing all methods and effectively subset HTTP (much more than today) I rather try and hold the line.

@opengeek

This comment has been minimized.

Copy link

@opengeek opengeek commented Feb 24, 2016

I was just trying to see if you would let go of the line for this one common method, but alas, I will go refactor their code to work without it.

@Lavoaster

This comment has been minimized.

Copy link

@Lavoaster Lavoaster commented Mar 23, 2016

@annevk I don't suppose you have any comment on github/fetch#243?

@mnot

This comment has been minimized.

Copy link
Member

@mnot mnot commented Mar 23, 2016

@annevk I wonder if it would help if devs got a console warning when case normalisation happened (to get them used to the idea that this is for backwards-compat, not something they can rely on for other methods).

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Mar 23, 2016

@Lavoaster what you're saying makes sense, I don't know why @dgraham uses lowercase method names which are supposed to be uppercase per HTTP. I filed #260 to add examples to Fetch to illustrate the difference.

@mnot I filed #259 on that.

annevk added a commit that referenced this issue May 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.