Declare generated GrpcType inheritance #2852
Conversation
Generated interface ProtoGrpcType is in fact an extension of grpc.GrpcObject, but this inheritance wasn't declared until now.
This forced users to cast `grpc.loadPackageDefinition(...): grpc.GrpcObject` first to `as unknown` and only then to `as {GeneratedGrpcType}`.
With this fix users will be able to cast to `as {GeneratedGrpcType}` directly.
There was a problem hiding this comment.
Please run npm run generate-golden in packages/proto-loader and update the example in the README.
| @@ -82,7 +82,7 @@ describe('Channelz', () => { | |||
| channelzClient = new channelzGrpcObject.grpc.channelz.v1.Channelz( | |||
| `localhost:${port}`, | |||
| grpc.credentials.createInsecure() | |||
| ); | |||
| ) as ChannelzClient; | |||
There was a problem hiding this comment.
If the purpose of this change is to avoid typecasts, why do we need to add one here? If you change the typecast on line 37 to the new suggested usage, would this one still be required?
There was a problem hiding this comment.
@murgatroid99 , fixed. Thank you!
enderteszla
force-pushed
the
enderteszla-patch-1
branch
3 times, most recently
from
f64867e
to
ab0c2ff
Compare
There was a problem hiding this comment.
Please revert the changes to the grpc-health-check package except for the ones that are direct consequences of the code generator change. In particular the published code should not interact with @grpc/grpc-js at all. This is a deliberate design decision to make the library compatible with both gRPC implementations. Modifications to that design can be discussed separately but they are out of scope for this change.
On a separate note, this change fully breaks compatibility with the old grpc package. That package does not export ServiceClient or ServiceClientConstructor types, and its grpc.GrpcObject is not compatible with the one this generator generates. That library still has 180K+ weekly downloads, so it's possible that this change will break people, and if we need to revert it that will be disruptive to a lot of people. If there is a way to avoid that breakage I would have a lot more confidence in publishing this change.
packages/proto-loader/README.md
Outdated
| const proto = (grpc.loadPackageDefinition( | ||
| packageDefinition | ||
| ) as unknown) as ProtoGrpcType; | ||
| const proto: ProtoGrpcType = grpc.loadPackageDefinition(packageDefinition); |
There was a problem hiding this comment.
In the modified usage in other files, as ProtoGrpcType appears to still be required, so that should also be specified here.
enderteszla
force-pushed
the
enderteszla-patch-1
branch
from
ab0c2ff
to
8ed659e
Compare
Generated interface ProtoGrpcType is in fact an extension of grpc.GrpcObject, but this inheritance wasn't declared until now.
This forced users to cast
grpc.loadPackageDefinition(...): grpc.GrpcObjectfirst toas unknownand only then toas {GeneratedGrpcType}.With this fix users will be able to cast to
as {GeneratedGrpcType}directly.Fixes #2851