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

feat(openapi-fetch): add onError handler to middleware #1974

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

luchsamapparat
Copy link
Contributor

@luchsamapparat luchsamapparat commented Oct 27, 2024

Changes

This PR adds an optional onError handler to the middleware feature. It allows handling errors thrown by fetch in three ways:

  • return nothing which means that the error will still be thrown, but useful for logging,
  client.use({
    onError({ error }) {
      console.error(error);
      return;
    },
  });
  • return another instance of Error to be thrown instead,
  client.use({
    onError({ error }) {
      return new Error("Oops", { cause: error });
    },
  });
  • or return a new instance of Response which means that the fetch call will proceed as successful.
  client.use({
    onError({ error }) {
      return Response.json({ message: 'nothing to see' });
    },
  });

How to Review

See additional tests in packages/openapi-fetch/test/middleware/middleware.test.ts

If multiple middlewares with onError are configured, they are executed in reverse order like onResponse. I'm not sure if that's intuitive or not. 🤔

Checklist

  • Unit tests updated
  • [-] docs/ updated (if necessary) 👈 I'll amend the commit with docs if this feature request is accepted
  • [-] pnpm run update:examples run (only applicable for openapi-typescript)

@luchsamapparat luchsamapparat requested a review from a team as a code owner October 27, 2024 11:56
Copy link

changeset-bot bot commented Oct 27, 2024

⚠️ No Changeset found

Latest commit: 1e2bfae

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@luchsamapparat
Copy link
Contributor Author

Before middleware was added to openapi-fetch, I created a wrapper for fetch to log requests, responses and failed requests (e.g. network errors, etc.). When migrating to the middleware approach, I wasn't able to implement logging for failed requests which is why I added that feature in this PR.

I could of course keep my fetch wrapper for handling failed requests, but that would spread the logging logic across two places.

Copy link
Contributor

@drwpow drwpow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this, thank you! I agree with your implementation of not throwing an error on any non-OK response, but just handling the instances where fetch() itself throws.

Tests look great, too!

Only things we need to merge this PR:

  • Update the documentation to mention this method (since docs update on merge). You don’t have to write much; just anything that mentions it exists, basically.
  • Add a patch changeset with npx changeset in the root (in 0.x, we’re using minor versions to indicate breaking changes, and this is not a breaking change)

@luchsamapparat luchsamapparat force-pushed the error-middleware branch 2 times, most recently from 0fb2e2f to 72dbb8a Compare November 10, 2024 20:01
@luchsamapparat
Copy link
Contributor Author

luchsamapparat commented Nov 10, 2024

I updated the docs. Feel free to make suggestions if something is missing or could be explained better somehow (or weird phrasing as I'm not a native speaker 😄).

Just to be sure: since this is a new feature, shouldn't it be a minor changeset instead of a patch?

@kerwanp kerwanp added the openapi-fetch Relevant to the openapi-fetch library label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openapi-fetch Relevant to the openapi-fetch library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants