feat: add decoded response to decodeExpecting errors #162
Conversation
There was a problem hiding this comment.
Looks fine to me, I don't see any breaking changes.
I quite dislike Errors and especially subclassing Errors, but decodeExpecting is designed to throw and there's no better value to throw than an Error.
Lastly, I want to make sure this change identifies the entirety of #137. I see
The error message would be more helpful if it included a stringified version of the decoded error response
as best I can tell this is the main ask, so looks like this PR hits the 🎯
|
Looks like a good improvement to me, although not exactly what I had in mind - do you think the "path-reported" decode error against the expected status code is clear enough to show the erroneous response from the API? I worry that the decode error will obfuscate it a little too much. I was hoping to see the value of the response. Perhaps we could just add that into the decode error? What do you think? |
|
Oh, I think I see what you mean. So instead of saying "I tried to decode 200Codec and got these errors", it's sufficient to say "I expected a 200 but got this status code and response instead"? The downside of the former being a lot of noise when a simpler response would convey the same information |
|
Maybe it would be helpful to list out the possibilities and what should happen:
Do these seem right? |
bitgopatmcl
force-pushed
the
add-original-decoded-request-to-superagent-error
branch
from
44d9f80 to
e9537c1
Compare
I hope we have tests for each case, I'll take a look Update: it appears we do |
There was a problem hiding this comment.
This change looks fine to me, it modifies decodeExpecting to return a particular error (DecodeError -- note @bitgopatmcl that this may be ambiguous with an io-ts decode error outside of an api-ts context) so we can identify this case with instanceof.
Additionally, change the error string in the message of the DecodeError to be more useful as determined by our users.
I dig it
| .catch((err) => (err instanceof DecodeError ? err.message : '')); | ||
|
|
||
| assert.isTrue(result); | ||
| assert.deepEqual(result, 'Could not decode response 200: {"invalid":"response"}'); |
There was a problem hiding this comment.
deepEqual an odd choice here!
| } else if (res.status === 'decodeError') { | ||
| const error = `Could not decode response ${String( | ||
| res.original.status, | ||
| )}: ${JSON.stringify(res.original.body)}`; |
There was a problem hiding this comment.
Note that JSON.stringify can also throw 😭 I believe when a circular reference is encountered.
fp-ts provides a wrapper that returns an Either, which we may want to use
There was a problem hiding this comment.
hurm how would you want to handle this case, @bitgopatmcl? It's pretty gnarly
I was about to open a ticket to track and then I had the thought -- is it even possible for res.original.body to contain a circular reference here? Meaning, was res.original.body recently JSON.parsed? If so, then I suppose there's no cause for concern (though I'll leave a note indicating that we've considered the unsafe use of JSON.stringify and determined that we're in the clear)
There was a problem hiding this comment.
What about if the server returns a non-application/json response? We should probably consider that case too.
| const error = `Unexpected response ${String( | ||
| res.original.status, | ||
| )}: ${JSON.stringify(res.original.body)}`; |
There was a problem hiding this comment.
Is res.original.status not already coerced to a string by the backticks?
| const error = `Unexpected response ${String( | |
| res.original.status, | |
| )}: ${JSON.stringify(res.original.body)}`; | |
| const error = `Unexpected response ${res.original.status}: ${JSON.stringify(res.original.body)}`; |
|
🎉 This PR is included in version 0.2.0-beta.13 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0-beta.18 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
|
🍾 thank you guys |
|
🎉 This PR is included in version 0.2.0-beta.7 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 0.2.0-beta.10 🎉 The release is available on npm package (@beta dist-tag) Your semantic-release bot 📦🚀 |
3 participants
Should address #137
Also it brings in
PathReporterwhich has a bug as per #138, but we were callingJSON.stringifyon the error anyway so it doesn't introduce a regression. Will try to sort that out in a follow-up.