Ky: TypeScript - Don't allow `body` option for GET and HEAD requests

[sindresorhus]

Would be nice to not allow the body option when using ky.get() and ky.head().

We could use a custom Omit type for this:

type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>

Would also be nice to handle ky(…, {method: 'get', body: …}), but not sure whether that is possible?

It would also be nice to disallow the body option when the json option is used.

More info: http://artsy.github.io/blog/2018/11/21/conditional-types-in-typescript/

[doniyor2109]

Hi. I tried to implement with conditional types but I think it is not possible use them in this context. If there was some generics then It would be easier to use conditional types. However I came up with union types. It works to but it is more verbose.

Here is PR for that #87

[SleeplessByte]

FWIW, at fuel we are running into servers now wanting GET requests with body.

Between RFC2616's obsoletion and RFCs 7230-7237 a few changes were made regarding this. Most importantly the following has been removed

RFC 216 Section 4.3
[...] if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request.

To my understanding this means that servers and peers may now accept bodies for GET (and HEAD) requests. The behaviour, just like with DELETE requests are not defined.

RFC 7231 Section 4.3.1 and Section 4.3.2
A payload within a GET request message has no defined semantics;
sending a payload body on a GET request might cause some existing
implementations to reject the request.

[sholladay]

servers now wanting GET requests with body

Do you have some examples? I believe you but it's hard to understand why anyone would be doing this.

[SleeplessByte]

Issues wanting GET and DELETE with bodies:
https://github.com/kittinunf/fuel/issues/567
https://github.com/kittinunf/fuel/issues/306
https://github.com/kittinunf/fuel/issues/245

From the spec:

The presence of a message body in a request is signaled by a
Content-Length or Transfer-Encoding header field. Request message
framing is independent of method semantics, even if the method does
not define any use for a message body.

Actual example in the wild:

https://www.elastic.co/guide/en/elasticsearch/guide/master/analysis-intro.html
(and an issue filed against this requirement for Postman: https://github.com/postmanlabs/postman-app-support/issues/131, and another one in swagger referencing elastic: https://github.com/swagger-api/swagger-ui/issues/2136)

[sholladay]

Thanks for those links. The ElasticSearch API is certainly a surprise to me. I might spend some time trying to convince them to make it a POST with a body or a GET with query parameters.

I'm inclined to say Ky should still disallow this as it's almost certainly a mistake. But with a popular API using it, that might become harder to justify. Ergh, this is why we can't have nice things!

[SleeplessByte]

Their search query allows both GET and POST, but I wouldn't try to "fix"
the internet as per the new specs it's not "broken".

TBH, this change normalises it as post and delete all also accept query
parameters (and now delete also a body).

On Mon, 21 Jan 2019, 05:43 Seth Holladay, [e-mail] wrote:

Thanks for those links. The ElasticSearch API is certainly a surprise to
me. I might spend some time trying to convince them to make it a POST
with a body or a GET with query parameters.

I'm inclined to say Ky should still disallow this as it's almost certainly
a mistake. But with a popular API using it, that might become harder to
justify. Ergh, this is why we can't have nice things!

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/sindresorhus/ky/issues/85#issuecomment-455946835, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB35WFXLb2MgsX_GPLLLfY1qJDcS0O-Gks5vFUWJgaJpZM4aJhja
.

[sholladay]

I'm aware that the spec technically allows it. But IMO that's not a good enough reason on its own. This is almost always going to be user error. I'd rather give the user clear and helpful feedback when they make a mistake than allow them to do something that's weird and unnecessary. If this causes genuine problems for people in an edge case, maybe we can come up with a pattern to do it using a custom instance or something so it's not too painful.

I'd say let's give this a try and see how it goes.

[SleeplessByte]

Yep, I understand that reasoning. I think if it's documented with the workaround (cast to any or to unknown and then to working) the reasoning holds up!

[A-Diddy]

Waiting for the day Mozilla wakes up and changes this to say GET requests have a body...

https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/GET

Until then, I'll be debating with myself which workaround is less wrong... (1) putting secret, complex queries into a publicly visible, URL-encoded query string parameter that could potentially exceed the URL size limit... or (2) sending requests for data with a POST method.

[sholladay]

Waiting for the day Mozilla wakes up and changes this to say GET requests have a body..

Maybe it should be updated to clarify that GET requests can technically have a body, but should not. The simplicity is nice, though.

Until then, I'll be debating with myself which workaround is less wrong... (1) putting secret, complex queries into a publicly visible, URL-encoded query string parameter that could potentially exceed the URL size limit... or (2) sending requests for data with a POST method.

A few thoughts:

1. Consider that URLs can actually be very long. Many thousands of characters, unless you need to support ancient browsers. See: https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers
2. Regarding complex queries, that sounds like a problem all on its own. Even in large apps with niche business logic, complex queries over HTTP are rarely a good idea. Think about why your client has to provide so much information to the server. Why doesn't your server have better knowledge of the desired "view" of the data to begin with? Not only do complex queries make your system more brittle, they are probably having an impact on performance. Consider having named collections for commonly requested slices of data. See if you can optimize your data structures so that more data can be fetched up front or in fewer requests, thus hopefully minimizing the URL-based querying you need to do. Optimize the names of each parameter, etc.
3. Consider sorting, and maybe filtering, more of the data on the client. I see a lot of people send a sort= parameter to the server for things that could really be sorted on the client. Unless the response is huge, this could even apply to filtering data. Depending on the workload, it may even be better for overall system performance if these CPU-intensive tasks are done on the client (and thus in a distributed manner) rather than a centralized server.
4. As for the URL being "publicly visible", well, the entire request is publicly visible by most definitions. Passing a query in the body doesn't encrypt it or anything. If your argument is that a URL-based query is visible in the address bar... well, that might apply to someone using form actions where the page reloads with a query, but that doesn't apply to anyone using Ky, as AJAX / fetch requests are only visible in the network tools.