-
Notifications
You must be signed in to change notification settings - Fork 56
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: cancel streaming grpc request when user ends stream #507
Conversation
Codecov Report
@@ Coverage Diff @@
## master #507 +/- ##
==========================================
- Coverage 97.85% 97.79% -0.06%
==========================================
Files 10 10
Lines 1303 1313 +10
Branches 345 347 +2
==========================================
+ Hits 1275 1284 +9
- Partials 28 29 +1
Continue to review full report at Codecov.
|
@@ -87,6 +87,9 @@ describe('Read Row Acceptance tests', function() { | |||
objectMode: true, | |||
}); | |||
|
|||
/* tslint:disable-next-line */ | |||
(stream as any).abort = function() {}; |
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 believe nodejs-common has an AbortableDuplex
interface that should work here.
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.
@JustinBeckwith could you take a look if I went about this the right way? I had to add a devDependency on common, and cast the PassThrough streams to Duplex
streams. It started to feel wrong :)
27c8bbc
to
0f9271e
Compare
package.json
Outdated
@@ -69,6 +69,7 @@ | |||
"through2": "^3.0.0" | |||
}, | |||
"devDependencies": { | |||
"@google-cloud/common": "^2.0.4", |
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.
Ugh. This will cause problems. Back the way it was! The any
will do fine.
LGTM if @JustinBeckwith is happy with it! |
This reverts commit 0f9271e.
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.
LGTM.
Fixes #506