-
Notifications
You must be signed in to change notification settings - Fork 370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(deps): Remove get-stream in favor of async iterators #1925
Conversation
src/file.ts
Outdated
@@ -4104,6 +4102,18 @@ class File extends ServiceObject<File> { | |||
this.storage.retryOptions.autoRetry = false; | |||
} | |||
} | |||
|
|||
private async getBufferFromReadable(readable: Readable): Promise<Buffer> { | |||
readable.once('error', e => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some difference in how node < 16 handles errors with streams / async iterators. Test cases were failing with Node 12 / 14 until I explicitly handled a stream error. However, node 16 handled it with just the iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broke other things... investigating a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, this block handled the error a bit differently - the throw wouldn't populate in getBufferFromReadable
, but would have a separate context as it is in a callback.
I actually don't think the catch is necessary - for await (const chunk of readable) {
should populate the throw. E.g. https://nodejs.org/api/stream.html#readablesymbolasynciterator
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1909 🦕