migrate database:get to new apiv2 client (with stream support!) #2781
Conversation
| @@ -238,6 +239,8 @@ export class Client { | |||
| let body: ResT; | |||
| if (options.responseType === "json") { | |||
| body = await res.json(); | |||
| } else if (options.responseType === "stream") { | |||
| body = (res.body as unknown) as ResT; | |||
There was a problem hiding this comment.
I don't think src/responseToError.js will properly handle stream bodies. Mind fixing that (with a test)?
There was a problem hiding this comment.
It... would JSON encode the stream object, which would just be weird. Until responseToError is TS'd, I would keep it as an exercise for the developer to not misuse the library (which we use in a correct way in this PR).
There was a problem hiding this comment.
I'd push back on this one. Since resolveOnHTTPError is false by default, it's really easy to write and test for the happy cases but hit the error cases sometimes, leaving a cryptic error message. Yes I see your comments below -- but would it make sense to throw if resolveOnHTTPError is not set to true with streaming for now and recommend manually handling all errors? We can figure out a story if we find more common usages.
There was a problem hiding this comment.
Alright - I'm throwing an error if (streaming && !resolvingOnHttpError), forcing the developer to have to set that and handle the error for now.
I'm open to future ideas on how to make responseToError better - especially when it comes to streaming!
bkendall
added
the
cleanup: request
Description
database:getcommand to the new apiv2Client.Scenarios Tested
database:get4xxresponse path)null, same as head).