Option to return non Response object from `ky` via `afterResponse`

[superr-fluous]

Hi! First of all, thanks for the lib, really like it so far!

As per docs for the afterResponse hook:

The return value of the hook function will be used by Ky as the response object if it's an instance of Response

I was wondering if there is a specific reason to always return a Response object and is it a viable idea to add an option to return just the value provided by the last afterResponse hook?

The use-case for this could be to de-serialize JSON at a single point in an app and possibly to add some additional data to the returned value (basically to narrow the scope of the returned data from network requests).

ky.create({
  hooks: {
    afterResponse: async (req, opt, res) => {
      if (res.status === 200) {
        return { ok: true, ...(await res.json()) };
      }
      return { ok: false, {/* some additional fields */} };
    },
  },
})

The issue with the current implementation would be the need to re-serialize data when creating a new Response object, thus creating unnecessary overhead, and, well, the fact that one would need to de-serealize it again.

Thanks for taking time to look at my issue! And my apologies in case I missed some info about my use-case in the docs or failed to spot a related existing issue.

[sholladay]

Hi and welcome. 🙂

It is not entirely clear to me what you want Ky to do with this object when it is returned. You want us to wrap it in a response and treat it as the response body?

Maybe #677 is what you are looking for?

[superr-fluous]

Thanks for the response!
For some reason I simply assumed that afterResponse is basically the last thing that Ky executes before returning the results of the response and that whatever I return from the hook callback is what I get when calling ky, ky.get, ky.post, ...etc. But I see now in the source code that it is not...

As to my intentions with this - I basically would like to be able to process the contents of a response during Ky's life-cycle and be able to specify the final return value. The motivation being:

1. Centralized JSON de-serialization.
2. Could aslo be used to handle request errors in a way that does not throw an Error (as beforeError does) but rather returns a certain value.
3. To make sure that Ky always returns a specific object. While we can use a generic ky<T>() to hint at the type, it is still nicer to have a JS-enforced data structure rather than TS typings.

// api.ts
export const api = ky.create({
  hooks: {
    // just an exemplary name
    beforeReturn: [
      // two cbs just to showcase the point
      await (req, opts, res) => ({ code: res.status, ...(await res.json()) }), // first cb to parse JSON
      (data) => ({ ok: data.code === 200, ...data }) // to add additional data
      // return of the last cb being the final return from Ky
    ],
  },
});

This way Ky always returns an object that contains ok and code.

// get.ts
type Res<T = Record<string | number | symbol, unknown> = T & {
  code: number;
  ok: boolean;
}

const res: Res = await api('url');
// no need to call res.json as it was already handled
if (res.ok) {
  // process normally
} else {
  // handle failed request
}

Why not use a wrapper?
While it is 100% working solution for this case, it is not as clean as using a hook - you'd need to create a wrapper for each Ky method, tinker with forwarding types from the wrapper to Ky and then use that wrapper everywhere instead of plain ky().

Why not use afterResponse to add fields to the returned Response object?
JSON de-serializing aside, I could use afterResponse, process the initial Response however I want and return whatever I want with a new Response object. The concern I have is the potential calculation overhead for large requests (1MB should be at around 10-15ms and scale linearly) - even though it could be disregarded is negligible there is also additional memory consumption and, well, it is an overhead that can be avoided theoretically.

Does this approach make sense or am I overcomplicating things? Thanks in advance for any response :)

EDIT: Forgot to mention - I took a look at #677 and it does not do what I'd like as far as I understand, since it's about a persistent context that is passed around during Ky's life-cycle

[sholladay]

1. Centralized JSON de-serialization.

That's what the stringifyJson (serialization) and parseJson (de-serialization) options are for. If those don't work for your use case, it would be helpful to understand why.

2. Could aslo be used to handle request errors in a way that does not throw an Error (as beforeError does) but rather returns a certain value.

If you mean that you want Ky to take your hook's return value and directly pass it along to the user who called ky(), then that is problematic because the user is expecting either a Response or an Error to be thrown, not some random object. And it seems counter to some of your goals, because then Ky cannot guarantee a return type since the hook could return whatever it wants. In fact, we would have to change our return type to any in that case, which would probably annoy a lot of people.

You may want to check out the throwHttpErrors option if you just want to handle non-2xx responses without an error being thrown.

3. To make sure that Ky always returns a specific object. While we can use a generic ky() to hint at the type, it is still nicer to have a JS-enforced data structure rather than TS typings.

Right now, the specific object that Ky always returns is an instance of Response. What you are proposing makes the return type less specific. I think it feels more specific to you because you're writing both the hook and the code that depends on it. But Ky has no way of knowing what is inside of your hook, what it will do, or what it will return.

Again, I may not be understanding how you want this to work. But it seems to me that automatically constructing a new response body from the hook's return value is the only improvement we could make here.

[superr-fluous]

Would like to preface, that at this point I propose not modifying the afterResponse hook but considering adding a new hook that would be called just before Ky's final return and, in case of non-void return from the hook call, returns data from the hook instead of a Response object

I see your point and there is but one moment I'd argue

Ky has no way of knowing what is inside of your hook, what it will do, or what it will return

While in terms of plain JS that is 100% true, in terms of TS it is possible to infer return type from the hook as well as to check it against provided return type via generic ky<T>(). Although I admit it is quite bothersome... still changing return type to any is not the only way... so it seems to me at least.

As to

Ky cannot guarantee a return type since the hook could return whatever it wants

My point here is that I guarantee the return type since I am writing the hook. And that's the entire point - that I strictly specify the return type via a hook.
Because, let's say, when handling responses in UI components, I'd like my returned data to always have the same structure in cases of an erroneous request { code: number, reason: string }. While I can make sure that my backend returns data that adheres to such a format, it is still possible to get network errors (HTTP 5xx), which ofc won't have the same structure.

automatically constructing a new response body

That's what I actually want to avoid since

the potential calculation overhead for large requests

that comes with processing the request and then constructing a new response.

As to parseJson - this way my custom JSON-parser will be attached to a Response object, I'd still need to call it in every handler that calls Ky.

const res = await ky();
const data = await res.json();

And a part of my reasoning is the ability to remove that boilerplate of calling .json() via a hook.

The same can be said about throwHttpErrors - while it won't throw an error, I'd still be getting a Response object, that I'd need to call .json upon.

If it helps to clarify my idea - I'd call this beforeReturn hook a way to back up the return type with JS in addition to passing a return type via a generic. Since the latter is basically a hint and does not provide any safety in browser.

I apologize if my wording can be confusing and some ideas seem unclear - not native speaker and do not get many opportunities to exercise conversing.

[sholladay]

... in terms of TS it is possible to infer return type from the hook as well as to check it against provided return type via generic ky()

True, but at the moment Ky's return types are the same no matter what options the user provides. Are you going to make two sets of types for of all of Ky's methods, one that applies when the hook is used and another for when the hook is not used? It sounds complicated. I'm open to seeing what this would look like in a PR, maybe I'm missing something and it can be done in a simple way. Could also be useful for #676.

that comes with processing the request and then constructing a new response.

Why do you believe the overhead of constructing a new response would be noticeable? I've yet to see an application where this would be a meaningful bottleneck. Make it work, then make it right, then make it fast. Let's focus on the semantics and ergonomics for now.

And a part of my reasoning is the ability to remove that boilerplate of calling .json() via a hook.

This sounds like #676. If that was implemented, would you still feel the need for a new hook?

I apologize if my wording can be confusing and some ideas seem unclear - not native speaker and do not get many opportunities to exercise conversing.

No worries, your English is relatively good and easy to understand. Some technical ideas are just hard to communicate, even between native speakers. 🙂

If you have time, you could make a PR that demonstrates the idea, even if it's just a proof of concept.

[superr-fluous]

Yeah , #676 would be nice.

would you still feel the need for a new hook

Depends... I still feel strongly about being able to enforce the structure of returned data. But it certainly gets closer and closer to being a complete ME problem :)

And yeah, the overhead is very very unlikely to be noticeable and even if there is a scenario there it would be a problem I doubt that Ky is what needs fixing in a scenario like that. Still it is a "it's still there" kind of concern for me, like "if it can be avoided, why not". Btw, thanks for sharing Make it work, then make it right, then make it fast. Haven't come across it before.

you could make a PR

Yeah, since I am using Ky for a pet project of sorts I was actually thinking of testing the idea at least. Hopefully I'll be able to the upcoming weekend. Although I sense that TS is going to be a headache :)

I'll try to update the issue so that it does not go stale