-
Notifications
You must be signed in to change notification settings - Fork 636
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
Typings issue in version 1.13.0 #425
Comments
@gunblues Would you mind sharing the output of |
@murgatroid99 I have reproduced this issue via https://github.com/gunblues/grpc-node-typing-issue |
This package, at the version 1.13, use protobufjs 5; so you should import @types/protobufjs@^5.x.x in you project and not the latest one. |
What's the story exactly when one uses |
As far as I see it, grpc-node is both compatible with protobufjs 5 and 6 (I use it with version 6). So neither version of grpc-node should not make any assumptions on the encoding format and thus should not depend directly on protobufjs, regardless of version. |
Yes, you're correct - what's your suggestion to do this properly ? |
On a high level, there should be an adapter object that is passed to the service description loading mechanism and that takes care of converting from raw buffers to Js objects and vice versa. This adapter would need to live in its own package and depend on its respective wire format library, so users will only drag in the dependencies necessary for their use case. So we would have packages Currently, Furthermore, we would need to get rid of any direct dependency on Actually, I think we are not too far away from that. The Btw. has anybody seen a project that uses gRPC with something other than protobuf? |
Is this suggestion substantially different from the As for removing the |
Yes, after looking at it, I think it is a step in the right direction, though it handles protobuf messages only, albeit both version 5 and 6 (I think). Would that mean, that the That would of course be a breaking change that needs a major version upgrade. |
The proto loader library specifically uses Protobuf.js 6. Protobuf.js 5 is outdated, and the As I said, we can't make breaking changes, so I wouldn't expect |
The problem is that If you are writing a gRPC server, the A possible solution is to remove the reference to Another stop gap solution everyone affected by this issue is to use a customized definition file. You need to configure your Typescript compiler to pick up this version for the {
"compilerOptions": {
"baseUrl": ".", // This must be specified if "paths" is.
"paths": {
"grpc": ["custom/grpc/definition/file"] // This mapping is relative to "baseUrl"
}
}
} |
I think I like the custom Message definition idea, but what exactly would it be like? I can't really think of anything else than something very generic like "object". |
Apologies for not looking into the Protobuf 5-6 tussle more when I introduced the change back to non-generic Would it be worthwhile to open a PR to change pb6 types to have a default |
That might be a good quick fix, yes. |
That PR has been merged and published, so this should no longer be a problem if you update |
Actually, it hasn't quite made it out yet. Reopening until then. |
And now it has :-D |
If I run tsc -d on my Typescript source I get the following message:
grpc-node version: 1.13.0
Typescript version: 2.9.2
tsconfig.json compiler options:
I have found this issue had been fixed before - #176
it had been restored back after this commit - abb05f0#diff-84a425dc635cd3214797bf694fad81e8
The text was updated successfully, but these errors were encountered: