-
Notifications
You must be signed in to change notification settings - Fork 524
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
Undici fetch
should at least warn on method: "patch"
since it does not normalize like http
#2294
Comments
fetch
should at least warn on method: "patch"
since it does not normalize like http
fetch
should at least warn on method: "patch"
since it does not normalize like http
@nodejs/undici should we transfer this to https://github.com/nodejs/undici? |
Two things:
In general I think documenting differences between node's http/s modules & maybe even node-fetch would be very useful |
Not sure, the issue with the warning is that it will last forever. I'd prefer to use it only for specific cases like notifying of the usage of experimental features, possible deprecations, etc. But for these behaviours, I'd prefer documentation changes as we follow the spec.
I agree with this one. The Undici's doc does not highlight this, which it should as it can be slightly problematic and hard to understand (I found myself in that spot several times) |
I don't think there should be a warning, but we should have a solid FAQ here, so that it's easily findable. |
in general fetch needs better documentation, I'd like to see the spec divergence section from the readme also put in the docs |
👍 Also just spent a while solving an issue where I'd specified a Searching for 'post request with undici' currently brings up this old issue: #570 instead of any recent docs/examples - I think a clearly signposted doc with examples of making common types of requests would be enough to solve the problem rather than runtime checks. |
I don't believe undici.request normalized the methods, this issue is referring to fetch. |
nodejs/node#51336 another issue regarding this 🫣
I think we should warn on this. The spec mentions that |
❤️
If I remember correctly from when I created this issue node-fetch does not normalize, but it calls into |
What is the problem this feature will solve?
I spent the better part of the morning debugging why outgoing requests starting failing for only our PATCH requests after moving to
fetch
fromhttp
for one of our internal tools.It ultimately was because undici (to be fair, following the spec) does not normalize
patch
toPATCH
unlike other common HTTP verbs.But this is not the behavior of http, which normalizes
patch
toPATCH
(all verbs, actually)Node itself will reject a request where the verb is
patch
, as all other servers and services I have tested.What is the feature you are proposing to solve the problem?
I propose node undici fetch emit a warning if the method "patch" is received as an option, informing the user that this HTTP method will not be normalized.
This would operate similar to how node emits (used to emit?) a warning if a promise rejection is not handled.
What alternatives have you considered?
I would frankly much rather prefer the old
http
behavior that normalizes all verbs, or at the very least makes an exception for PATCH, as the decision of a service to supportpatch
with different semantics thanPATCH
strikes me as an extremely uncommon use case.That said, I understand undici in particular favors specification compliance, so emitting a warning will be helpful in the short term as more folks like me move projects over to the native API.
The text was updated successfully, but these errors were encountered: