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

Undici fetch should at least warn on method: "patch" since it does not normalize like http #2294

Closed
pckilgore opened this issue Sep 27, 2023 · 9 comments · Fixed by #2577
Closed
Labels
Docs Changes related to the documentation fetch

Comments

@pckilgore
Copy link

pckilgore commented Sep 27, 2023

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 from http for one of our internal tools.

It ultimately was because undici (to be fair, following the spec) does not normalize patch to PATCH unlike other common HTTP verbs.

But this is not the behavior of http, which normalizes patch to PATCH (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 support patch with different semantics than PATCH 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.

@pckilgore pckilgore changed the title fetch should at least warn on method: "patch" since it does not normalize like http Undici fetch should at least warn on method: "patch" since it does not normalize like http Sep 27, 2023
@tniessen
Copy link
Member

@nodejs/undici should we transfer this to https://github.com/nodejs/undici?

@KhafraDev KhafraDev transferred this issue from nodejs/node Sep 27, 2023
@tniessen tniessen added enhancement New feature or request fetch labels Sep 27, 2023
@KhafraDev
Copy link
Member

KhafraDev commented Sep 27, 2023

Two things:

  • adding a warning is fine, I have no problems with it.
  • fetch shouldn't be treated as a 1:1 alternative to http, and maybe we should just document these differences instead.

In general I think documenting differences between node's http/s modules & maybe even node-fetch would be very useful

@metcoder95
Copy link
Member

adding a warning is fine, I have no problems with it.

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.

In general I think documenting differences between node's http/s modules & maybe even node-fetch would be very useful

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)

@mcollina
Copy link
Member

I don't think there should be a warning, but we should have a solid FAQ here, so that it's easily findable.

@KhafraDev
Copy link
Member

in general fetch needs better documentation, I'd like to see the spec divergence section from the readme also put in the docs

@metcoder95 metcoder95 added Docs Changes related to the documentation and removed enhancement New feature or request labels Sep 28, 2023
@autopulated
Copy link
Contributor

👍 Also just spent a while solving an issue where I'd specified a {method:'post'} instead of POST to undici.request, getting me a cloudflare 400 from an API, so it seems that other verbs are sometimes not normalised too?

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.

@KhafraDev
Copy link
Member

I don't believe undici.request normalized the methods, this issue is referring to fetch.

@KhafraDev
Copy link
Member

nodejs/node#51336 another issue regarding this 🫣

  • node-fetch and Safari normalize the method (wrong)
  • Chrome & Firefox & undici do not (correct)

I think we should warn on this. The spec mentions that patch is unlikely to succeed and it's a frequent issue for people moving over from node-fetch (as demonstrated by the number of bug reports about this that we've received). It's also confusing behavior: delete, get, head, options, post, and put are all normalized, but patch isn't. From the issue I linked, it also looks like an issue with undici, rather than being intentional behavior.

@pckilgore
Copy link
Author

pckilgore commented Jan 3, 2024

❤️

  • node-fetch and Safari normalize the method (wrong)

If I remember correctly from when I created this issue node-fetch does not normalize, but it calls into node:http which does (this issue was the result of me moving node-fetch code to undici/global).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Changes related to the documentation fetch
Projects
Status: Pending Triage
Development

Successfully merging a pull request may close this issue.

6 participants