Add support for generating service client implementations with JSON serialization #106
Conversation
There was a problem hiding this comment.
@isherman I think this looks great, had a few questions about option flag names / Rpc/JsonRpc.
Out of curiosity, what server-side would this end up talking to? ...Twirp but JSON instead of binary? I suppose it's somewhat agnostic given the Rpc interface, but still curious so that I can best understand how this would be used.
| @@ -248,6 +248,8 @@ protoc --plugin=node_modules/ts-proto/protoc-gen-ts_proto ./batching.proto -I. | |||
|
|
|||
| - With `--ts_proto_opt=outputClientImpl=false`, the client implementations, i.e. `FooServiceClientImpl`, that implement the client-side (currently Twirp-only) RPC interfaces will not be output. | |||
|
|
|||
| - With `--ts_proto_opt=outputJsonClientImpl=true`, an additional client implementation will be output for each service, i.e. `FooServiceJsonClientImpl`, that uses JSON serialization. Has no effect if `outputClientImpl=false`. | |||
There was a problem hiding this comment.
What do you think about doing something like outputClientImpl = false | true | twirp | json? I.e. false is off, true is a legacy value that means twirp, and json would be your new code?
I'm kinda thinking that shops will generally have a single preferred client?
I dunno, maybe not I guess?
Maybe this is better.
Maybe outputJsonClient=true/false? I might deprecate the existing outputClientImpl and rename it to outputTwirpClient...
| describe('simple service', () => { | ||
| it('can echo with protobuf serialization', async () => { | ||
| const echoRpc = { | ||
| request: (_service: string, _method: string, data: Uint8Array) => { |
There was a problem hiding this comment.
Ah sure, that makes sense. How about using jest.fns though so we can assert the right values are passed? I.e. expect(...).toBeCalledWith?
| request: (_service: string, _method: string, data: Uint8Array) => { | ||
| return Promise.resolve(data); | ||
| }, | ||
| requestJson: (_service: string, _method: string, data: unknown) => { |
There was a problem hiding this comment.
I was initially confused by this Rpc having to have both request and requestJson ... kinda thinking we should just generate separate Rpc interfaces, i.e. the current Rpc stays as is and becomes BinaryRpc and the new one would be a JsonRpc?
| } | ||
| rpc = rpc.addFunction(generateRpcRequestFn({ ...options, outputJsonClientImpl: false })); | ||
| if (options.outputJsonClientImpl) { | ||
| rpc = rpc.addFunction(generateRpcRequestFn(options)); |
There was a problem hiding this comment.
I.e. here maybe just make a new JsonRpc that is separate from the existing Rpc type...
|
Closing as stale, and in favor of the discussion about a more general approach happening here: #106 |
First pass at #7.
Doesn't include support for Batching yet. Any other features this interacts with that I should be aware of?
Thoughts on this approach?