feat(fetch): add ability to throw on error

[AllieJonsson]

Status

READY

Description

Fixes #2047

When using a package like swr or query, they use exceptions to know if the response is an error or not. To accomplish this, I added the shouldThrowOnError setting. When set to true, the function is typed to return only the success type (or success types, if there are multiple), when set to false, the behavior is unchanged.

When the endpoint does not have an error specified:

export type listPetsResponse200 = {
  data: Pets;
  status: 200;
};

export type listPetsResponseSuccess = listPetsResponse200 & {
  headers: Headers;
};

export const listPets = async (
  params: ListPetsParams,
  options?: RequestInit,
): Promise<listPetsResponseSuccess> => {
  const res = await fetch(getListPetsUrl(params), {
    ...options,
    method: 'GET',
  });

  if (!res.ok) {
    const err: globalThis.Error & { info?: any; status?: number } =
      new globalThis.Error();
    const body = [204, 205, 304].includes(res.status) ? null : await res.text();
    const data = body ? JSON.parse(body) : {};
    err.info = data;
    err.status = res.status;
    throw err;
  }

  const body = [204, 205, 304].includes(res.status) ? null : await res.text();
  const data: listPetsResponseSuccess['data'] = body ? JSON.parse(body) : {};
  return {
    data,
    status: res.status,
    headers: res.headers,
  } as listPetsResponseSuccess;
};

When the endpoint does have an error specified:

export type createPetsResponse200 = {
  data: Pet;
  status: 200;
};

export type createPetsResponseDefault = {
  data: Error;
  status: Exclude<HTTPStatusCodes, 200>;
};

export type createPetsResponseSuccess = createPetsResponse200 & {
  headers: Headers;
};
export type createPetsResponseError = createPetsResponseDefault & {
  headers: Headers;
};

export const createPets = async (
  createPetsBody: CreatePetsBody,
  params: CreatePetsParams,
  options?: RequestInit,
): Promise<createPetsResponseSuccess> => {
  const res = await fetch(getCreatePetsUrl(params), {
    ...options,
    method: 'POST',
    headers: { 'Content-Type': 'application/json', ...options?.headers },
    body: JSON.stringify(createPetsBody),
  });

  const body = [204, 205, 304].includes(res.status) ? null : await res.text();
  if (!res.ok) {
    const err: globalThis.Error & {
      info?: createPetsResponseError['data'];
      status?: number;
    } = new globalThis.Error();
    const data: createPetsResponseError['data'] = body ? JSON.parse(body) : {};
    err.info = data;
    err.status = res.status;
    throw err;
  }

  const data: createPetsResponseSuccess['data'] = body ? JSON.parse(body) : {};
  return {
    data,
    status: res.status,
    headers: res.headers,
  } as createPetsResponseSuccess;
};

[soartec-lab]

@AllieJonsson Thanks for the improvement.

In addition to this case, there is a case where error handling is done in custom-fetch. There are many ways to handle error handling, such as controlling specific responses individually so that exceptions are not thrown.

https://github.com/orval-labs/orval/blob/master/samples/next-app-with-fetch/custom-fetch.ts#L52-L53

The issue I originally reported was simply to limit the type definition of response data to 200.

---

Although the scope is different from the first issue, there was also a request to add default error handling.

https://github.com/orval-labs/orval/issues/1839

I support this response because I think it will benefit them.

[melloware]

Look like merge conflicts?

[AllieJonsson]

@AllieJonsson Thanks for the improvement.

In addition to this case, there is a case where error handling is done in custom-fetch. There are many ways to handle error handling, such as controlling specific responses individually so that exceptions are not thrown.

https://github.com/orval-labs/orval/blob/master/samples/next-app-with-fetch/custom-fetch.ts#L52-L53

The issue I originally reported was simply to limit the type definition of response data to 200.

Although the scope is different from the first issue, there was also a request to add default error handling.

#1839

I support this response because I think it will benefit them.

I completely forgot about custom fetch. I assume when shouldThrowOnError is true, we should just type it as the success response, and let the customFetch handle the error? Then maybe the name of the setting is wrong. Maybe forceSuccessResponse is better? Is there a need for the customFetch method to know about the error type in this case?

So many questions... 😄

export type createPetsResponse200 = {
  data: Pet;
  status: 200;
};

export type createPetsResponseDefault = {
  data: Error;
  status: Exclude<HTTPStatusCodes, 200>;
};

- export type createPetsResponseComposite =
-   | createPetsResponse200
-   | createPetsResponseDefault;

- export type createPetsResponse = createPetsResponseComposite & {
-   headers: Headers;
- };

+ export type createPetsSuccessResponse = createPetsResponse200 & {
+   headers: Headers;
+ };
+ 
+ export type createPetsErrorResponse = createPetsResponseDefault & {
+   headers: Headers;
+ };

export const createPets = async (
  createPetsBodyItem: CreatePetsBodyItem[],
  options?: RequestInit,
- ): Promise<createPetsResponse> => {
-   return customFetch<createPetsResponse>(getCreatePetsUrl(), {
+ ): Promise<createPetsSuccessResponse> => {
+   return customFetch<createPetsSuccessResponse>(getCreatePetsUrl(), {
    ...options,
    method: 'POST',
    headers: { 'Content-Type': 'application/json', ...options?.headers },
    body: JSON.stringify(createPetsBodyItem),
  });
};

[soartec-lab]

@AllieJonsson

This is great. Thank you for these questions as they are very valuable to me 🙌 So I agree with the sample code you provided.

I also think the option name forceSuccessResponse is more appropriate than shouldThrowOnError. shouldThrowOnError would be a good name to add an option to define the default exception throw handling as an alternative.

[melloware]

Looks like merge conflicts need to be resolved.

[pedrosimao]

Is this PR stuck? Does the author plans to solve the conflicts?

I am also facing a similar problem, where I have a customFetch function that throw errors. After the 7.4+ Orval version, all the returned types of our app were broken. As I am trying to solve the issue I found this PR, but I tend to believe there could be a better solution.

I think while we are using external clients or customFetch we should allow this customFetch function itself to be the source of truth of which type is being returned, instead of forcing a type.

For instance, the code generated should be like this:

export const getAllPosts = async (params?: GetAllPostsParams, options?: RequestInit) => {
  
  return customFetch<getAllPostsResponse>(getGetAllPostsUrl(params),
  {      
    ...options,
    method: 'GET'
    
    
  }
);}

Instead of this:

export const getAllPosts = async (params?: GetAllPostsParams, options?: RequestInit): Promise<getAllPostsResponse> => {
  
  return customFetch<getAllPostsResponse>(getGetAllPostsUrl(params),
  {      
    ...options,
    method: 'GET'
    
    
  }
);}

Removing : Promise<getAllPostsResponse> would allow customFetch to return any type it wants, and not only throwing or not throwing errors.

Would anyone agree or disagree with my view? If there is some disagreement on this being the better design choice, we could add an option named something like allowCustomReturnType on the mutator options. Not sure if the same would be applicable to custom clients like swr and so, since we don't use it.

[AllieJonsson]

Is this PR stuck? Does the author plans to solve the conflicts?

I am also facing a similar problem, where I have a customFetch function that throw errors. After the 7.4+ Orval version, all the returned types of our app were broken. As I am trying to solve the issue I found this PR, but I tend to believe there could be a better solution.

I think while we are using external clients or customFetch we should allow this customFetch function itself to be the source of truth of which type is being returned, instead of forcing a type.

For instance, the code generated should be like this:

export const getAllPosts = async (params?: GetAllPostsParams, options?: RequestInit) => {
  
  return customFetch<getAllPostsResponse>(getGetAllPostsUrl(params),
  {      
    ...options,
    method: 'GET'
    
    
  }
);}

Instead of this:

export const getAllPosts = async (params?: GetAllPostsParams, options?: RequestInit): Promise<getAllPostsResponse> => {
  
  return customFetch<getAllPostsResponse>(getGetAllPostsUrl(params),
  {      
    ...options,
    method: 'GET'
    
    
  }
);}

Removing : Promise<getAllPostsResponse> would allow customFetch to return any type it wants, and not only throwing or not throwing errors.

Would anyone agree or disagree with my view? If there is some disagreement on this being the better design choice, we could add an option named something like allowCustomReturnType on the mutator options. Not sure if the same would be applicable to custom clients like swr and so, since we don't use it.

Sorry, I've been on vacation and in parental leave and haven't been in the office for several months, I'm looking to pick up on this pr in a couple of weeks (hopefully before the next release 😄).

Yes, it seems like removing the response type in the function should work, but I think we only would want to do this for customFetch, maybe possibly we should do it as a setting

[Odonno]

I agree that if the customFetch handles error, we should be able to export a Promise of the type of the 200 data type, otherwise it should naturally default to the basic response type.

[pedrosimao]

Yes, it seems like removing the response type in the function should work, but I think we only would want to do this for customFetch, maybe possibly we should do it as a setting

Hi @AllieJonsson , thanks for replying, even during your parental leave. Really amazing that you look into this.

I am not sure about all libraries, because I haven't tested them, but if we are adding this allowCustomReturnType as an option, then I feel like it would probably be useful for any external client library, and not only customFetch. Most of these libraries probably do expect a generic type and return a customized version of this generic data type anyway... not forcing a type into something that will probably evolve, and which is out of our control, seems like a better design choice overall to me.

[ptlthg]

I just started evaluating using Orval in my own projects, and the ability to customize the return type here would be a great help, so +1 to the conversation going on here.

I did notice that setting output.override.fetch.includeHttpResponseReturnType to false removes all the custom return type generation and just adds : Promise<SuccessDataType>, but I still would rather have it use the return type of my custom fetch function.

[soartec-lab]

@MattFromGer @Odonno @pedrosimao @ptlthg @AllieJonsson

Thanks everyone for the discussion. Let me clarify the discussion. There are three confusing points here.

1. The shouldThrowOnError option for allowing exception throwing to be customized within fetch.

As the PR title suggests, this is the first thing they're trying to address.

2. The forceSuccessResponse option limits the return type to success only.

This is what I suggested in the original issue, and @AllieJonsson is also working on it since the problem still persists.

3. Allowing generic types to be injected into the response type.

This is what they're proposing in the latest discussion.

---

Do these three provide enough proposals for discussion? If there are no issues, we'll focus on 1 or 2 here and move any new proposals to a separate issue.

[AllieJonsson]

Hi @AllieJonsson If it's completed please let me know 👍

I think its completed. I recall there was something I wanted to change, but I cant remember what it was 🤔

[soartec-lab]

And if we run into any issues, you will probably remember what you forgot, so we can add it then 👍

[AllieJonsson]

Hi, @AllieJonsson Thanks, this is a great change. Could you please add an explanation of the new options to the documentation?

This is the thing I was thinking of adding! 😅 I'll try to get it done this weekend

[soartec-lab]

@AllieJonsson Oh year, I'm glad you remembered!

[AllieJonsson]

@AllieJonsson Oh year, I'm glad you remembered!

@soartec-lab I think I'm done now, please check if you think there are anything more to change/add. :)