refactor: add dependency on @grpc/proto-loader#229
Conversation
src/grpc.ts
Outdated
| load( | ||
| filename: GrpcLoadFileArg|GrpcLoadArgs, _?: null, | ||
| options?: GrpcLoadOptions): grpc.GrpcObject { | ||
| if ((Array.isArray as (x) => x is GrpcLoadArgs)(filename)) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/grpc.ts
Outdated
| filename = filename[0] as GrpcLoadFileArg; | ||
| } | ||
| let root: string|null = null; | ||
| if (typeof filename !== 'string') { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/grpc.ts
Outdated
| options = filename[2] || filename[1] || {}; | ||
| filename = filename[0] as GrpcLoadFileArg; | ||
| } | ||
| let root: string|null = null; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
test/grpc.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| // TODO: Re-evaluate what the contract of loadProto is. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
19f92ac to
a1ad021
Compare
|
|
||
| it('should load the test file', () => { | ||
| const protos = grpcClient.loadProto(TEST_PATH, TEST_FILE); | ||
| // tslint:disable-next-line:no-any |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Any updates on the status of this refactor? |
|
@kinwa91 Sorry, this flew under my radar. Seems like things are failing due to a change I made in gRPC types recently, so I'll need to go update the dependencies. |
|
We were blocked by a PBJS TypeScript change. Now that it's been released, hopefully tests should pass now. |
Fixes googleapis/google-cloud-node-core#462
This PR removes usages of
grpc.loadandgrpc.loadObject, replacing them with the new@grpc/proto-loaderpackage.As it stands currently, this will not pass tests. This is because
loadProtoasks a lot of its return value -- it must be a gRPC client, but it also must haveprotobufjs-related fields on it. The new implementation ofloadintentionally moves away from this interface because of its dependency on the surface types ofprotobufjs. Therefore, I think the tests that check the return value ofloadProtoshould be re-evaluated.I'm in the process of testing to make sure client libraries still have the same behavior. In the meantime, I wanted to start a discussion on the
loadProtoissue here.