refactor(errors) increases verbosity of error messages to help debugging #122
Conversation
MattGson
force-pushed
the
master
branch
3 times, most recently
from
6300cb7 to
1bf8264
Compare
| @@ -91,7 +91,9 @@ export const schemaFromConfluentSchema = ( | |||
| break | |||
| } | |||
| default: | |||
| throw new ConfluentSchemaRegistryArgumentError('invalid schemaType') | |||
| throw new ConfluentSchemaRegistryArgumentError( | |||
| `invalid schemaType ${(confluentSchema as ConfluentSchema).type}`, | |||
There was a problem hiding this comment.
Why would you need to cast this when confluentSchema is already a ConfluentSchema according to the function parameter definition?
| @@ -4,6 +4,8 @@ interface ConfluenceResponse extends Omit<Response, 'data'> { | |||
| data: () => { | |||
| message: string | |||
| } | |||
| responseData?: unknown | |||
| errors?: unknown[] | |||
There was a problem hiding this comment.
I don't see the errors field on the Response type:
There is an error field, but no errors. There is an errors property in the JS object, but since it's not exposed in the types, I would take that to mean it's a private property that we shouldn't be accessing.
Same with the responseData field. There is a rawData method that's exposed that we could use, but I'm not sure why we'd both access data and rawData, and not just the former.
There was a problem hiding this comment.
In fact, I think this type can be replaced with just referencing the Response type directly, since this type doesn't actually do anything meaningful. The data method is already a generic that allows you to define the return type.
| @@ -0,0 +1,6029 @@ | |||
| { | |||
There was a problem hiding this comment.
This project uses yarn and should not have an npm lockfile committed.
| @@ -1,13 +1,13 @@ | |||
| import avro from 'avsc' | |||
There was a problem hiding this comment.
I don't see this being used anywhere.
| @@ -12,7 +14,14 @@ class ResponseError extends Error { | |||
| url: string | |||
|
|
|||
| constructor(clientName: string, response: ConfluenceResponse) { | |||
| super(`${clientName} - ${response.data().message || `Error, status ${response.status()}`}`) | |||
| super( | |||
There was a problem hiding this comment.
This change has caused some tests to fail that need to be updated.
The final string now has a trailing space if there are no errors. I would suggest the following instead:
`Error, status ${response.status()}${response.error() ? `: ${response.error()}` : ''}`
I've added some more details into error messages where I got tripped up.
I ended up digging through the source code to debug which isn't ideal.
Let me know if you have any feedback.
fixes #123