Error handling of getStream/createStream/etc. #208
Conversation
… the promise if there is an error
timoxley
left a comment
timoxley
left a comment
There was a problem hiding this comment.
Some nitpicking and suggestions to improve test robustness, but otherwise lgtm.
| } catch (err) { | ||
| return ErrorCode.UNKNOWN | ||
| } | ||
| const code = json.code |
There was a problem hiding this comment.
Yeah follow the eslint warning here, use object destructuring
| const keys = Object.keys(ErrorCode) | ||
| if (keys.includes(code)) { | ||
| return code as ErrorCode | ||
| } |
There was a problem hiding this comment.
Using in operator should get around the need to add the as ErrorCode type assertion.
return code in ErrorCode ? code : ErrorCode.UNKNOWN| errorCode?: ErrorCode | ||
|
|
||
| constructor(message: string, response: Response, body?: any) { | ||
| constructor(message: string, response?: Response, body?: any, errorCode?: ErrorCode) { |
There was a problem hiding this comment.
Maybe instead of passing errorCode in, just have it read it from the body since it's right there?
| @@ -0,0 +1,20 @@ | |||
| export enum ErrorCode { | |||
There was a problem hiding this comment.
IMO this file's contents should probably live in authFetch.ts.
| return new Stream(this.client, json) | ||
| } | ||
|
|
||
| async getOrCreateStream(props: { id?: string, name?: string }) { |
There was a problem hiding this comment.
This should probably be:
props: { id: string } | { name: string }then
if ('id' in props) {
// etc
}| }) | ||
|
|
||
| it('invalid id', () => { | ||
| return expect(() => client.createStream({ id: 'invalid.eth/foobar' })).rejects.toThrow() |
There was a problem hiding this comment.
I don't always add them myself, but should probably put something in toThrow, even if it's just a partial string match, otherwise should your failure pathway have a programmer error e.g. entirely possible it's throwing on a ReferenceError or SyntaxError, this test could be mistakenly seeing that instead of an actual expected error.
| public: false, | ||
| }) | ||
| return json[0] ? new Stream(this.client, json[0]) : undefined | ||
| return json[0] ? new Stream(this.client, json[0]) : Promise.reject(new AuthFetchError('', undefined, undefined, ErrorCode.NOT_FOUND)) |
There was a problem hiding this comment.
Ahh I see why you left errorCode as a parameter. This '', undefined, undefined, is pretty terrible though.
Maybe could wrap that messiness up and expose an API like:
AuthFetchError.NotFound = class NotFoundError extends AuthFetchError {
constructor(msg = '') {
super(AuthFetchError(msg, undefined, undefined, ErrorCode.NOT_FOUND))
}
}
new AuthFetchError.NotFound(`Could not find stream by name: ${name}`)Then later you could do if (err instanceof AuthFetchError.NotFound) instead of if (err instanceof AuthFetchError) && (err.errorCode === ErrorCode.NOT_FOUND)
uh, though I guess this would be misleading since other non-AuthFetchError.NotFound errors could have err.errorCode === ErrorCode.NOT_FOUND, so you'd need a factory that creates the appropriate error instance:
export function createAuthFetchError(msg, response, body) {
switch (parseErrorCode(body)) {
case ErrorCode.NOT_FOUND: return new AuthFetchError.NotFound(msg, response, body)
case ErrorCode. VALIDATION_ERROR: return new AuthFetchError.ValidationError(msg, response, body)
default: return new AuthFetchError(msg, response, body)
}
}Just a suggestion though, might not be worth it, your call.
createStream,getStream,getStreamByNameandgetOrCreateStreamreject the promise if there is an API error.To support this functionality, a new
errorCodefield was added to AuthFetchError. Currently the ErrorCode enum lists the most typical API error codes:VALIDATION_ERRORandNOT_FOUND. Other codes can be added later, if needed.