-
Notifications
You must be signed in to change notification settings - Fork 32
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 in full cause to thrown errors #94
Conversation
6a56058
to
189cbf4
Compare
@courcelan can you update the commit message to use |
removes auth_token from reporting update tests
341b027
to
6ede5e4
Compare
done |
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.
lib/useButter.js
Outdated
@@ -123,18 +123,38 @@ export default function useButter(type, butterConfig) { | |||
detail: errorDetail |
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.
In some cases error detail might be not in detail
prop. Should we cover somehow or current state is ok?
https://share.zight.com/8LuNRWmL
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.
thanks for this @marcinpece-apptension, I've updated the code to handle a more general set of error keys/values. Please give this all another check.
const errors = await errorPayload.response.json(); | ||
|
||
const mappedParams = Object.fromEntries( | ||
Object.entries(errorPayload.params) | ||
.filter(([key]) => key !== 'auth_token') | ||
); | ||
|
||
const url = new URL(errorPayload.response.url); | ||
|
||
// Remove auth_token from URL search params | ||
url.searchParams.delete('auth_token'); | ||
|
||
const cause = { | ||
data: errors, | ||
headers: errorPayload.response.headers, | ||
status: errorPayload.response.status, | ||
statusText: errorPayload.response.statusText, | ||
config: errorPayload.config, | ||
params: mappedParams, | ||
type, | ||
url | ||
}; |
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.
💚
@@ -129,7 +129,7 @@ test( | |||
catch (error) { | |||
await expect(error).toBeDefined(); | |||
await expect(error).toBeInstanceOf(Error); | |||
await expect(error.message).toEqual("Error: Not found"); | |||
await expect(error.message).toEqual("Error: Not found (404)"); |
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.
I have one error while running tests https://share.zight.com/L1uEo6mm
@ViolanteCodes Has the error in the test been fixed before merge? |
@marcinpece-apptension yes. I caught it in #95 and updated there prior to you mentioning it. so we should be all good. |
Cool 🎉 Thanks |
this PR (for issue #93) updates the reporting for a thrown Error.
Errors now report with the following:
err.message
: the detail of the error and status codeerr.cause
: info on what was passed to the APIerr.cause
contains:data
: the detail of the errorheaders
: any headersstatus
: response status codestatusText
: any response status textconfig
: the configuration used in the fetchparams
: any parameters passed to the requestype
: the content type requestedurl
: object contain theURL
schemaPlease note that any reference to
auth_token
is removed from the resulting cause.