-
Notifications
You must be signed in to change notification settings - Fork 435
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
client: don't call callbacks if close() is called #258
client: don't call callbacks if close() is called #258
Conversation
Worth noting that when I built this PR into my application, the errors I was experiencing went away. |
This seems reasonable to me, but I'm also not a typescript developer 😂. I've rerun the failing job, the CI tests are a bit flaky unfortunately. |
This seems to be failing consistently https://travis-ci.org/improbable-eng/grpc-web/jobs/441938330, though I doubt it's related to this change:
|
Given that it passes everywhere else, I think it might just be a test configuration error. I can't force this through myself but have tried alerting others so we can get this in. |
Ping. |
Just had a look into the build failure, I believe the root cause is the failure to bring up Chrome:
We are seeing the same failure on master so I am also confident this failure is unrelated to the changes. I'll merge this through, however we should follow up with fixing this particular browser test, or remove it from the matrix |
- ts-protoc-gen clears out the callbacks anyways - Fixes improbable-eng#257
77f2ba4
to
fa7bd30
Compare
Looks like it passed. 👍 |
LGTM! |
... I'd write a test for this, but I'm not actually a Javascript/Typescript developer so I'm not familiar with the test framework.