Resolve issues that popped up with grpc-js 1.10.x series #609
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Per discussion in deeplay-io#590 this version of yarn is known working
…ures seem to match the output in deeplay-io#555 - work debug from here
…nce the 'cancelled' event is called after ever RPC regardless of success outcome, I modified nice-grpc to track when an RPC completes. Once an RPC completes, I prevent the abort signal from being able to be triggered. (2) corrected how errors are propagated back from server to client by using call.emit('error', ...) instead of call.destroy(...)
…tly telling developers what is allowed. In the past I'd seen this before and it wasn't immediately obvious to me what had happened.
…de v18+ and I believe we only had the forced compat for v14 or v16 which required --experimental-abortcontroller
…for node we only support node versions with abort-controller anyhow
…n when we discovered 1.10.x was broken) - tested locally with node v18.20.3 working at this point with "resolutions": {"@grpc/grpc-js": "1.9.5"}, and also with resolutions pinned to 1.10.8 -- all tests pass, but it's worth noting that nice-grpc-web is still slow (just over 5min), as it always appears to have been on my PC
updated .nvmrc back to 18.x series and explicitly updated @types/node to v18.x series as well.
…execution) that nodejs internals which we were previously accessing via an `as any` cast are indeed in the approximate shape that we expect.
|
Latest snapshot ought to have everything required to close out #555 I didn't test this hypothesis due to time, but based on discussion in grpc/grpc-node#2681 (comment) it is possible that nice-grpc will perform inconsistently in grpc-js v1.10.0 and v1.10.1 My related questions to the grpc-js team are in: grpc/grpc-node#2771 I'd simply advise people to skip those versions out of safety. |
aikoven
reviewed
Jun 6, 2024
…entially incompatible 1.10.0 and 1.10.1 (2) document new tests (3) use existing library defer-promise in tests
aikoven
approved these changes
Jun 6, 2024
|
Thanks @davidfiala! I will publish a release right away. |
|
Published 📦 |
|
Just to clarify something since I've spent a day or two digging, would this fix rule out the problem I've highlighted in another library that utilise nice-grpc? The link is above in the mentioned reference. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tested on linux with node v20.14.0 and grpc-js left as-is as well as when grpc-js was pinned to 1.10.8 via
resolutionsin package.json.unfortunately my workstation hangs on
nice-grpc-server-health:testandnice-grpc-web:testso I'm curious to see what the CI pipeline does with themshould fix #607 and #555