Remove use of deprecated grpc.load() method #1669
Conversation
rsgowman
force-pushed
the
rsgowman/grpc_load
branch
from
ec74af8 to
f9feb6d
Compare
rsgowman
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
Rather than separate tests for encoding/decoding. Similar to the other platforms.
rsgowman
force-pushed
the
rsgowman/grpc_load
branch
from
f9feb6d to
c5e87f0
Compare
|
Ready for re-review. This is a meld of this plus #1699. Note that the upgrade to protobufjs 6.x required a minor change to packages/firestore/src/remote/serializer.ts... it seems as though protobufjs now always returns this as a unit8array? But my knowledge of our typescript port in general is a little thin, so definitely take a close look to make sure that makes sense during your review. I've been a little paranoid on the types (i.e. |
|
Sorry for the slow review on this, but there are a couple things I'm trying to figure out and I think I need to spend a bit of time debugging through this. I'm not sure why serializer.ts needed to be changed. And I think our hasTag() code is not being used like it was before and can perhaps be removed. I'll poke at this more Monday and get back to you. |
1. The switch to protobufjs 6.x seems to have removed the need for hasTag() (and actually it had broken our usage of it since the tags ended up changing from snake_case to camelCase). So I've removed hasTag() and set `oneofs: false` in protoLoaderOptions so the virtual "fooType" fields are no longer added. 2. 'seconds' in timestamps should always be serialized as a string, since it's an int64 but we weren't doing that. I fixed it, which removed some of the special casing from rsgowman/grpc_load. 3. I reworked the expect...RoundTrip methods into a single unified one which saves some repetition and I think is easier to make sense of. 4. I re-added the disabled useProto3Json assert and instead tweaked the offending test not to try to round-trip through protobufjs.
| @@ -1364,35 +1354,3 @@ export class JsonProtoSerializer { | |||
| return FieldMask.fromArray(fields); | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This also requires a workaround to keep our tests passing.