8000 feat(fetch): add ability to throw on error by AllieJonsson · Pull Request #2168 · orval-labs/orval · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(fetch): add ability to throw on error #2168

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(fetch): add ability to throw on error #2168

wants to merge 4 commits into from

Conversation

AllieJonsson
Copy link
Contributor

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;
};

@AllieJonsson AllieJonsson requested a review from soartec-lab June 17, 2025 08:57
@AllieJonsson AllieJonsson force-pushed the feat/throw-on-error branch from 0baf8ec to 04a85e9 Compare June 17, 2025 09:24
@soartec-lab soartec-lab self-assigned this Jun 17, 2025
@soartec-lab
Copy link
Member

@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.

@melloware
Copy link
Collaborator

Look like merge conflicts?

@AllieJonsson
Copy link
Contributor Author

@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
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soartec-lab soartec-lab Awaiting requested review from soartec-lab

At least 1 approving review is required to merge this pull request.

Assignees

@soartec-lab soartec-lab

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow fetchclient's data to be set to success type only
3 participants
@AllieJonsson @soartec-lab @melloware
0