-
Notifications
You must be signed in to change notification settings - Fork 323
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
Comments
No, |
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 |
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 |
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. |
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.) |
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 It's about consistency and common usage. |
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. |
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. |
See marmelab/restful.js#81 and the related issue at JakeChampion/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. |
What did you rely on before? You had some abstraction that uppercased everything? Or did you rely on a bug in certain browsers where |
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. |
As I mentioned, we'd break projects that are using HTTP correctly and want to use |
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. |
@annevk I don't suppose you have any comment on JakeChampion/fetch#243? |
@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). |
This issue should be reopened. I landed here because this reddit thread shows more developers are getting tripped up by this.
@annevk In your own words, "Do you have some data to back that up?" I.e. Do you have any examples of projects that rely on such a distinction? I'd be genuinely surprised if such exist. The vast majority of developers expect HTTP methods to be uppercase or case-independent. For those few that are aware of the distinction, relying on the fact "patch" is the one standard method that's case-sensitive would seem extremely ill-advised. Any code leveraging this quirky behavior would have to be considered an inelegant hack, at best (imho). I'm with the other commenters here - I don't understand the current rationalization for maintaining this. Yes, the HTTP spec says methods are case-sensitive, but the Fetch authors have clearly decided to ignore that by normalizing the standard HTTP methods. Leaving "patch" as the odd one out makes no sense. |
The normalization described at https://fetch.spec.whatwg.org/#concept-method-normalize is for a select set of methods that have been normalized as such since browsers started allowing custom HTTP methods. Rationale is detailed below in notes and examples. |
To support @annevk as a different implementer: Among the many values shared by different browser vendors, two of the most important driving factors are:
Adhering to standards promotes interoperability and provides a predictable experience for users that, no matter the browser the user chooses, the web will work consistently. Not breaking existing content is, among other things, about ensuring that no matter the browser version the user uses, the web will work consistently. Now, these obviously aren’t absolute; for example, browsers will diverge from standards and legacy compatibility to protect user privacy and security, when appropriate. But they are two important pillars that changes are assessed against. “Does this change the API” and “Is this spec compatible” are used to evaluate most changes. Now, in this case, there is spec divergence in Fetch from the HTTP spec, by having it normalize. This is non-ideal: it is not spec compliant (with HTTP). But, for better or worse, it reflects what was needed to avoid breaking legacy content because bugs in implementation quickly become assumptions by web developers. You can see that yourself, as you mention that despite HTTP specifying things are case-sensitive, developers have come to rely on assuming it was case insensitive due to historic bugs in browsers. That’s unfortunate, because it means you can write code for web browsers that won’t work in non-browser contexts, because only Web browsers have this “quirk”. Historically, this “quirk” only applied to certain methods. Other methods were left, as both Fetch and HTTP specify, case-sensitive. That was the API guarantee of older browsers, and that’s the API guarantee of other platforms. Normalizing I understand the argument here is developer ergonomics: developers (incorrectly, at least according to the specs) expect more normalization, because they’ve come to see the bugs for the other methods as features. In the Priority of the Constituencies, that’s certainly an important consideration, and is even weighed more than browser manufacturer concerns or spec purity. But that doesn’t mean it’s absolute or guaranteed to trumpet everything, but rather, it is weighed against the tradeoffs. @annevk has rightly captured that, based on the currently available information, the current behavior is the right tradeoff, even if it may be surprising to some developers. The risks, of both breaking old code and of spec interoperability, are significant enough that even if absolute breakage isn’t quantified, as one of the foremost experts in making these tradeoff decisions, this is one where we know there is meaningful risk. The value proposition of making it easier for developers is important, but for such an issue like this, with available workarounds, it’s not appropriate to break old content and remove developer control (as normalizing would). |
This CL adds tests for https://fetch.spec.whatwg.org/#concept-method-normalize to check that the methods not listed there are not uppercased. This CL also adds more complete test coverage around case-sensitiveness of request methods. Discussion for not uppercasing "patch" method: whatwg/fetch#50 Bug: 1228178 Change-Id: I003cd65f0c330cc5b4641d79dce721970795c994
This CL adds tests for https://fetch.spec.whatwg.org/#concept-method-normalize to check that the methods not listed there are not uppercased. This CL also adds more complete test coverage around case-sensitiveness of request methods. Discussion for not uppercasing "patch" method: whatwg/fetch#50 Bug: 1228178 Change-Id: I003cd65f0c330cc5b4641d79dce721970795c994 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3032582 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#926658}
This CL adds tests for https://fetch.spec.whatwg.org/#concept-method-normalize to check that the methods not listed there are not uppercased. This CL also adds more complete test coverage around case-sensitiveness of request methods. Discussion for not uppercasing "patch" method: whatwg/fetch#50 Bug: 1228178 Change-Id: I003cd65f0c330cc5b4641d79dce721970795c994 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3032582 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#926658}
This CL adds tests for https://fetch.spec.whatwg.org/#concept-method-normalize to check that the methods not listed there are not uppercased. This CL also adds more complete test coverage around case-sensitiveness of request methods. Discussion for not uppercasing "patch" method: whatwg/fetch#50 Bug: 1228178 Change-Id: I003cd65f0c330cc5b4641d79dce721970795c994 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3032582 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#926658}
…only Automatic update from web-platform-tests [WPT] Fetch method normalization This CL adds tests for https://fetch.spec.whatwg.org/#concept-method-normalize to check that the methods not listed there are not uppercased. This CL also adds more complete test coverage around case-sensitiveness of request methods. Discussion for not uppercasing "patch" method: whatwg/fetch#50 Bug: 1228178 Change-Id: I003cd65f0c330cc5b4641d79dce721970795c994 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3032582 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#926658} -- wpt-commits: 14f36e0dd4932d1c2a7dcfc541e1ba89e512a788 wpt-pr: 29680
…only Automatic update from web-platform-tests [WPT] Fetch method normalization This CL adds tests for https://fetch.spec.whatwg.org/#concept-method-normalize to check that the methods not listed there are not uppercased. This CL also adds more complete test coverage around case-sensitiveness of request methods. Discussion for not uppercasing "patch" method: whatwg/fetch#50 Bug: 1228178 Change-Id: I003cd65f0c330cc5b4641d79dce721970795c994 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3032582 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#926658} -- wpt-commits: 14f36e0dd4932d1c2a7dcfc541e1ba89e512a788 wpt-pr: 29680
This CL adds tests for https://fetch.spec.whatwg.org/#concept-method-normalize to check that the methods not listed there are not uppercased. This CL also adds more complete test coverage around case-sensitiveness of request methods. Discussion for not uppercasing "patch" method: whatwg/fetch#50 Bug: 1228178 Change-Id: I003cd65f0c330cc5b4641d79dce721970795c994 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3032582 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#926658}
This CL adds tests for https://fetch.spec.whatwg.org/#concept-method-normalize to check that the methods not listed there are not uppercased. This CL also adds more complete test coverage around case-sensitiveness of request methods. Discussion for not uppercasing "patch" method: whatwg/fetch#50 Bug: 1228178 Change-Id: I003cd65f0c330cc5b4641d79dce721970795c994 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3032582 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#926658} NOKEYCHECK=True GitOrigin-RevId: 14db9b01056c0180f6e29bb9286d4e1cfe6e14f7
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
The text was updated successfully, but these errors were encountered: