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

Breaking change: deprecate body on http methods with no defined body semantics #2357

Closed
pxue opened this issue Oct 18, 2023 · 8 comments
Closed
Labels
question [Use a Discussion instead] semver-major Features or fixes that will be included in the next semver major release

Comments

@pxue
Copy link
Contributor

pxue commented Oct 18, 2023

Extending discussion from #2305 (comment)

if there's no explicit body, or it is a method whose body's semantics are not defined, set content-length: 0

This will be a breaking change:

  • All shouldSendContentLength=false methods will no longer parse and send any attached body
  • content-length will be forced to 0 on request
@metcoder95
Copy link
Member

To give more context around it:

The discussion was going around not allowing to attach bodies to those requests whose doesn't have defined semantics to handle the body of a given request (e.g. GET, OPTIONS, etc.).
Currently undici allows these cases, this issue was more to discuss wether we should allow it or now.

Having second thoughts about it

It might be good to keep it this way, as anyways it is up to the server to decide what to do in those scenarios.

Thoughts?

cc @mcollina @ronag @KhafraDev

@metcoder95 metcoder95 added bug Something isn't working question [Use a Discussion instead] semver-major Features or fixes that will be included in the next semver major release and removed bug Something isn't working labels Oct 22, 2023
@mcollina
Copy link
Member

dispatch should allow it. fetch should follow the spec. I'm on the fence about request.

@metcoder95 metcoder95 removed the bug Something isn't working label Oct 23, 2023
@metcoder95
Copy link
Member

Over a quick search, it seems fetch spec does not say what to do exactly in these scenarios (unless I'm overlooking it), just specifies redirects. Although Chrome throws when attempting to.

For dispatch I agree, for request I think we can simplify our lives by just keeping it as is; the changes done by #2305 can be the kickoff and keep an eye on it.

@Ethan-Arrowood
Copy link
Collaborator

If Fetch spec isn't explicit, then try to copy Chrome and other browser's behavior.

@KhafraDev
Copy link
Member

The fetch spec has very little to do with http (the rfcs are mentioned as reference points for implementations). We should instead follow either what the rfcs say or what's best for node.

@metcoder95
Copy link
Member

Then the question remains open:

From what I've seen, both browsers (firefox and chrome) throw when attempting to pass a body on a GET request; we can follow their lead and do the same or keep it as is.

At much, I believe undici should remain untouched but maybe fetch aligns with browsers

@KhafraDev
Copy link
Member

that part is from the fetch spec, which undici implements. I also think undici should stay the same. If anyone feels differently then this issue can be re-opened.

@Ethan-Arrowood
Copy link
Collaborator

Agreed - undici can do what it wants HTTP wise if things aren't specified. Fetch should strictly follow spec and browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question [Use a Discussion instead] semver-major Features or fixes that will be included in the next semver major release
Projects
None yet
Development

No branches or pull requests

5 participants