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

Add support for error handling in Orval built in fetch #1839

Open
oferitz opened this issue Jan 21, 2025 · 6 comments
Open

Add support for error handling in Orval built in fetch #1839

oferitz opened this issue Jan 21, 2025 · 6 comments
Labels
fetch Fetch client related issue

Comments

@oferitz
Copy link

oferitz commented Jan 21, 2025

I understand that this issue has been discussed here: #1387, and the suggested solution is to create a custom fetch.

However, creating a custom fetch can sometimes be an overhead and introduce unwanted side effects, as highlighted in this issue: #1838.

Given these challenges, I believe it would be reasonable to consider adding support for error handling directly in Orval's built-in fetch. This enhancement would simplify the developer experience and avoid the need for custom implementations in many cases.

Your consideration of this feature would be much appreciated!

@melloware melloware added the fetch Fetch client related issue label Jan 21, 2025
@soartec-lab
Copy link
Member

@oferitz
I understand that you want to add the following to the source code automatically generated by orval. Is my understanding correct?

if (!response.ok) {
  throw new Error('Network response was not ok')
}

Ref: https://tanstack.com/query/latest/docs/framework/react/guides/query-functions#usage-with-fetch-and-other-clients-that-do-not-throw-by-default

@oferitz
Copy link
Author

oferitz commented Jan 26, 2025

I want to preserve the original error so something like:

fetch('https://api.example.com/data')
  .then(async response => {
    if (!response.ok) {
      const errorData = await response.json();
      // Preserve the original status and error details
      const error = new Error(errorData.message || response.statusText);
      error.status = response.status;
      error.data = errorData;
      throw error;
    }
    return response.json();
  })
  .catch(error => {
    // Handle both network errors and response errors
    console.error('Status:', error.status);
    console.error('Message:', error.message);
    console.error('Data:', error.data);
  });

@soartec-lab
Copy link
Member

@oferitz
Thanks. The implementation of error handling varies depending on the project. Therefore, we are not thinking about supporting one case with orval.

@oferitz
Copy link
Author

oferitz commented Feb 1, 2025

@soartec-lab Thanks for your feedback. I understand your point about implementation variations. Let me propose a more standardized approach that strictly follows the Response API specification.
The following example only uses standard Response API properties (status and statusText), making it more universal and aligned with web standards. This approach:

  • Preserves the essential error information from the Response object
  • Follows standard practices for HTTP error handling
  • Doesn't make assumptions about error response body structure

This minimal implementation could serve as a foundation that users can extend based on their specific needs. Would you consider supporting this basic level of error handling in Orval?

// Basic error handling aligned with Response API spec
fetch('https://api.example.com/data')
  .then(async response => {
    if (!response.ok) {
      // Using only standard Response API properties
      const error = new Error(response.statusText);
      error.status = response.status;
      throw error;
    }
    return response.json();
  })
  .catch(error => {
    // Handle errors with standard Response properties
    console.error(`${error.status}: ${error.message}`);
  });

@soartec-lab
Copy link
Member

@oferitz

I also understand the argument that it improves convenience.
However, the decision to throw an exception itself is left to the user's discretion. Therefore, I didn't think this should be supported as a minimal implementation.

@oferitz
Copy link
Author

oferitz commented Feb 2, 2025

@soartec-lab

That's a valid point—you want to allow the user to choose whether or not to throw an error. I totally get it.

For this, I have one last suggestion for your consideration. If it's not accepted, I'll go ahead and close the issue 🙂.

My suggestion is to add the ability to throw an error, as described in my basic level of support above, but to make it configurable via Orval. This would work similarly to how users can choose whether to include the HTTP response return type.

  override: {
        fetch: {
          includeHttpResponseReturnType: false,
         // New suggested config
          throwOnException: true
        }
   }

Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetch Fetch client related issue
Projects
None yet
Development

No branches or pull requests

3 participants