+better stream decoder #16
Conversation
|
LGTM could you write tests for this? |
| @@ -16,25 +14,56 @@ export function throwError( | |||
| } | |||
| } | |||
|
|
|||
| // deno-lint-ignore no-explicit-any | |||
| export async function decodeStream( | |||
| export async function decodeStream<T>( | |||
There was a problem hiding this comment.
Nice change, a fan of this
| ) { | ||
| const chunks = res.body! |
There was a problem hiding this comment.
First of all, thank you for the PR, we really appreciate it!
I think my big issue with this PR is the unnecessary complexity it introduces here. If I understand correctly, you have two issues with the current implementation:
- There's an error that we're not catching
- Wanting to be able to cancel the stream
Both of these can be solved in like ~10 lines of well written code with the current implementation as a base. Reimplementing a stream decoder seems not very useful to me (though I'd love to be proven wrong)!
If you would be willing to rework this PR, please let me know! If not, I will go through and fix these myself. Have a nice day!
There was a problem hiding this comment.
I wanna rework this. About the tests. Where should I put them?
There was a problem hiding this comment.
We don't really have tests per se since it's pretty hard to enable testing without using the API (we just use examples right now).
| ) { | ||
| const chunks = res.body! |
There was a problem hiding this comment.
We don't really have tests per se since it's pretty hard to enable testing without using the API (we just use examples right now).
| } | ||
| // console.log(data.error); | ||
| throw new Error(errorMessage); | ||
| export function throwErrorIfNeeded(response: unknown) { |
There was a problem hiding this comment.
Could you split this off into another PR? Ref: #18
There was a problem hiding this comment.
Should we restore the function code and create a new PR with this code and the issue reference?
I don't wanna create a branch from this branch. Should i wait until this be merged?
Sorry, I'm not an expert on github. I don't know the best practices for the prs T-T
There was a problem hiding this comment.
Should we restore the function code and create a new PR with this code and the issue reference?
Yes, this would be great!
| .getReader(); | ||
|
|
||
| try { | ||
| for (;;) { |
There was a problem hiding this comment.
for (;;) { is really dubious to me. Why not while(true) {?
The previous stream decoder got me "chunks is not async iterable" error. I wrote a better Stream Decoder. It ables to cancel the stream whenever you want.