Handle error response types in spec #37
Conversation
Api.ts
Outdated
| params: RequestParams = {} | ||
| ) => | ||
| this.request<RackResultsPage, any>({ | ||
| this.request<RackResultsPage, Error>({ |
There was a problem hiding this comment.
seems simple right
generator/gen-client.ts
Outdated
| .filter(isTruthy) | ||
| .map(refToResponseName) | ||
| // need to dedupe because 4xx and 5xx are the same for now | ||
| .reduce(uniq, [] as string[]); |
There was a problem hiding this comment.
feeling smart as hell for this one
There was a problem hiding this comment.
I either want like a do() function just to run something on the whole array or the pipe operator. It'd make this moot.
There was a problem hiding this comment.
Sadly looks like pipeline operator is not moving much. We might get pipe() sooner than that, and we can just write our own if we want that.
https://github.com/tc39/proposal-pipeline-operator/blob/main/HISTORY.md
https://github.com/js-choi/proposal-function-pipe-flow
generator/gen-client.ts
Outdated
| export function uniq<T>(acc: T[], next: T): T[] { | ||
| if (!acc.includes(next)) { | ||
| acc.push(next); | ||
| } | ||
| return acc; | ||
| } |
There was a problem hiding this comment.
It might not be a big deal given it'd probably be used with small input sizes, but I believe this operates in quadratic time. For every N items in the list you're having to do N comparisons to verify if the thing is in the list or not.
There was a problem hiding this comment.
not worried about it yet because N = 2, plus it's only happening at codegen time, not even build time
| } | ||
|
|
||
| if (!r.ok) throw r; | ||
| return r; |
There was a problem hiding this comment.
this stuff doesn't change any functionality, I just thought it was weird that we were mixing promises and async/await
|
|
||
| export type ApiResponse<Data extends unknown> = | ||
| | SuccessResponse<Data> | ||
| | ErrorResponse; |
There was a problem hiding this comment.
now we can actually discriminate between the two
Added to spec in oxidecomputer/dropshot#286 and integrated into Omicron in oxidecomputer/omicron#710. First commit is setup; the meat is in 7e6c428.
Originally (see intermediate commits if you dare) this was meant to be fancy and add per-endpoint error response types. However, the whole point of the way Dropshot is written is that the error type is the same on all endpoints and is not generic, which saves us a world of pain on the console side. So instead all I really did was get rid of the error type parameter entirely.
HttpResponsetoApiResponseApiResponsea discriminated union of success and errorrequest<Instance, Error>