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(createError): inherit fields from cause as fallback #795

Merged
merged 5 commits into from
Jul 7, 2024

Conversation

jerelmiller
Copy link
Contributor

Resolves #691

Updates the logic for detecting the statusCode from an error by checking if it can be found on error.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's ApolloError 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.

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" &&
Copy link
Contributor Author

@jerelmiller jerelmiller Jun 27, 2024

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.

Copy link
Member

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);
Copy link
Contributor Author

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?

@pi0
Copy link
Member

pi0 commented Jul 7, 2024

Thanks for PR dear @jerelmiller i will push some refactors to generalize it.

(also note, current main branch is targeting upcoming h3 v2, if more ecosystem backward compatibility adoption is needed, we might want to backport it to v1 as well)

@pi0 pi0 changed the title Allow parsing statusCode from error.cause feat(createError): inherit field from cause as fallback Jul 7, 2024
@pi0 pi0 changed the title feat(createError): inherit field from cause as fallback feat(createError): inherit fields from cause as fallback Jul 7, 2024
Copy link

codecov bot commented Jul 7, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.82%. Comparing base (a58d7c9) to head (0c75e01).
Report is 120 commits behind head on main.

Files Patch % Lines
src/error.ts 92.30% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@pi0
Copy link
Member

pi0 commented Jul 7, 2024

Thanks again for PR and sorry for late review. I have pushed few refactors, simplifying implementation and also we use cause to inherit statusCode, statusMessage and fatal/unhandled fields (all which could be masked with wrapper)

@pi0 pi0 merged commit 4d67f19 into unjs:main Jul 7, 2024
3 checks passed
@jerelmiller jerelmiller deleted the parse-status-from-cause branch July 7, 2024 20:05
@jerelmiller
Copy link
Contributor Author

Wonderful. Thanks so much for the review and getting this in there!

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.

Allow for parsing of error status codes from non-http response errors.
2 participants