-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix(types): augment types to manage response streams #2188
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2188 +/- ##
=======================================
Coverage 84.31% 84.31%
=======================================
Files 9 9
Lines 1594 1594
Branches 129 110 -19
=======================================
Hits 1344 1344
Misses 250 250 Continue to review full report at Codecov.
|
GlobalOptions, | ||
BodyResponseCallback, | ||
APIRequestContext, | ||
} from 'googleapis-common'; | ||
import {Readable} from 'stream'; |
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.
[suggestion, feel free to ignore]
should this be imported only if used by the given API? (otherwise, unused imports)
I must admit I have no idea if this applies to APIs in this library, but in GAPICs, we do put all such imports inside if statements in templates.
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.
Yeah, in this particular case the import is only being used for the Readable
type. It could be required for any of the API calls, if the user uses {responseType: 'stream'}
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.
I don't think I totally understand the actual changes, but I understand the problem it's solving!
Fixes #2052, and adds a lot more TypeScript 😬