fix: add callback close condition #837
Conversation
hersentino
changed the title
| ${ | ||
| !useAbortSignal | ||
| ? `return client.close();` | ||
| : `if (!abortSignal || !abortSignal.aborted) |
There was a problem hiding this comment.
Ah gotcha; @hersentino it looks like we don't have an integration/ test that generates the !abortSignal || !abortSignal.aborted case? If not, do you mind adding one? Just like copy/paste the grpc-web directory to like grpc-web-abort-signal and add the abort signal flag to parameters.txt I think?
No need to actually have a runtime test suite that covers this behavior, b/c I assume that is hard to test (unless it's easy to test, then feel free! :-), but at least having some generated code checked in that shows this code being generated will help us keep the behavior from regressing.
Thanks!
There was a problem hiding this comment.
it looks like we don't have an integration/ test that generates the !abortSignal || !abortSignal.aborted case?
We have one, it is located in integration/grpc-web-no-streaming-observable, but since it does not contain streaming-observable and my fix is specifically related to streaming, there was no difference when I ran bin2ts.
do you mind adding one?
@stephenh Tell me what you prefer, keeping it in integration/grpc-web-no-streaming-observable or creating a new one?
There was a problem hiding this comment.
It was quick, so I added it
There was a problem hiding this comment.
That's great, thanks @hersentino !
hersentino
force-pushed
the
fix-add-callback
branch
from
accaee8 to
c05cb39
Compare
## [1.148.1](v1.148.0...v1.148.1) (2023-05-25) ### Bug Fixes * add callback close condition ([#837](#837)) ([2071c67](2071c67))
|
🎉 This PR is included in version 1.148.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In order to solve #836