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: Wrap AxiosErrors in custom error object #151

Merged
merged 17 commits into from
Jul 25, 2024

Conversation

joe-yeager
Copy link
Contributor

@joe-yeager joe-yeager commented Jun 17, 2024

Description and Context

I think our http functions could be considered a leaky abstraction because we are directly exposing AxiosErrors which requires the projects depending on these functions to write axios specific logic in their code base, such as checking e.response && e.response.status in catch blocks. What I am proposing is that we globally wrap all AxiosErrors in our own custom Error to keep the implementation details limited to this repository.

This would mean that error checking could be simplified from something like:

try {
   await someFunctionThatMakesAnHttpCall();
} catch (e) {
  if(isHubSpotHttpError(e). && e.status === 404) {
     console.log('Unable to find the thing you were looking for')
  }
}

So the resulting code is completely unaware that axios is being used under the hood.

Who to Notify

@brandenrodgers @camden11 @kemmerle

@camden11
Copy link
Contributor

This is a really good idea - we should absolutely be giving consuming libraries everything they need to interface with the errors we throw here. Should definitely be very careful to make sure we don't lose any info contained in the errors along the way, but I don't really see a downside to doing this

@joe-yeager
Copy link
Contributor Author

joe-yeager commented Jun 18, 2024

Should definitely be very careful to make sure we don't lose any info contained in the errors along the way

@camden11 💯

My understanding is that by creating the new error type and setting the error thrown by axios as cause we preserve all of the stack trace context. We can also still access the axios error fields in the event there is a piece of information we don't have available on the top level of the new error.

@joe-yeager joe-yeager marked this pull request as ready for review June 20, 2024 17:38
@joe-yeager joe-yeager changed the title POC: Wrap AxiosErrors in custom error object feat: Wrap AxiosErrors in custom error object Jun 20, 2024
http/index.ts Outdated Show resolved Hide resolved
errors/apiErrors.ts Outdated Show resolved Hide resolved
errors/apiErrors.ts Outdated Show resolved Hide resolved
errors/apiErrors.ts Outdated Show resolved Hide resolved
@joe-yeager joe-yeager marked this pull request as draft June 21, 2024 17:24
@joe-yeager joe-yeager changed the base branch from main to jy/switch-error-handlers-generic June 21, 2024 20:13
@joe-yeager joe-yeager changed the base branch from jy/switch-error-handlers-generic to next July 17, 2024 19:51
Copy link
Contributor

@kemmerle kemmerle left a comment

Choose a reason for hiding this comment

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

Besides the one tiny nit, this all looks great to me. Particularly like how you made the request fields top level in HubSpot errors. Chef's kiss👩‍🍳 💋

export function getAxiosErrorWithContext(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
error: AxiosError<any>,
export function getHubSpotHttpErrorWithContext(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just recording here that this will be a major version bump since it's a breaking change (totally fine, just wanted to add a comment for posterity)

@joe-yeager joe-yeager merged commit 87497dd into next Jul 25, 2024
@joe-yeager joe-yeager deleted the jy/POC-Wrap-Axios-Errors branch August 28, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants