-
Notifications
You must be signed in to change notification settings - Fork 225
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(createError): inherit fields from cause
as fallback
#795
Conversation
src/error.ts
Outdated
@@ -121,6 +121,13 @@ export function createError<DataT = unknown>( | |||
err.statusCode = sanitizeStatusCode(input.statusCode, err.statusCode); | |||
} else if (input.status) { | |||
err.statusCode = sanitizeStatusCode(input.status, err.statusCode); | |||
} else if ( | |||
typeof input.cause === "object" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since cause
can have all sorts of shapes, I find it best to do some type checking here before we try and parse from it. This is a bit verbose though compared to the first two conditional checks, so if you prefer more terseness, let me know and I'd be happy to pull this out to an external helper function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline check is fine only cause
can be null
too (also saw it in apollographql/apollo-client#11902). typeof null === "object"
and well next line fails so we can simplify it to truehy checking like input.cause?.statusCode
.
src/error.ts
Outdated
"statusCode" in input.cause && | ||
typeof input.cause.statusCode === "number" | ||
) { | ||
err.statusCode = sanitizeStatusCode(input.cause.statusCode, err.statusCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also try and detect status
from error.cause.status
since this is also checked on the top-level properties, or should I just check statusCode
here?
Thanks for PR dear @jerelmiller i will push some refactors to generalize it. (also note, current |
statusCode
from error.cause
cause
as fallback
cause
as fallbackcause
as fallback
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #795 +/- ##
==========================================
+ Coverage 77.83% 85.82% +7.98%
==========================================
Files 47 44 -3
Lines 4286 4958 +672
Branches 611 673 +62
==========================================
+ Hits 3336 4255 +919
+ Misses 933 701 -232
+ Partials 17 2 -15 ☔ View full report in Codecov by Sentry. |
Thanks again for PR and sorry for late review. I have pushed few refactors, simplifying implementation and also we use |
Wonderful. Thanks so much for the review and getting this in there! |
Resolves #691
Updates the logic for detecting the
statusCode
from an error by checking if it can be found onerror.cause
. This helps better detect a status code from an error created by a library that creates their own wrapped errors (such as Apollo Client'sApolloError
class).See apollographql/apollo-client#11634 for more context on this change. We've opened apollographql/apollo-client#11902 on our end to make this integration possible.