-
Notifications
You must be signed in to change notification settings - Fork 347
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
Generate a JSON client #7
Comments
I think ideally it would be possible to pass a protoc option to specify which wire formats to generate. This would be nice because JSON-only clients then wouldn't need to import the JS protobuf package. |
@alecthomas yeah, that is a good point. We'd only needed protobuf/twirp for our rpc/services, so was all we've implemented so far... The service impl code is fairly straightforward: https://github.com/stephenh/ts-proto/blob/master/src/main.ts#L725 And I think really it'd just be https://github.com/stephenh/ts-proto/blob/master/src/main.ts#L695 Would you be interesting in tacking a crack at this? I might be able to hack on this at some point, but all the better if you'd like to, and then could verify the impl / usage / etc. within your project / ecosystem. |
I started to take a look at what would be involved here. Unfortunately, protobuf.js does not support encoding/decoding to/from canonical proto3 JSON: protobufjs/protobuf.js#1304 It looks like it's partially implemented, but lacking support for important well-known-types like And google's JS protobuf library has no support at all: protocolbuffers/protobuf-javascript#95 Do you see any way forward? |
It looks like there are some open PRs that would help, e.g. protobufjs/protobuf.js#1258 Maybe the way forward is to implement this using protobufjs, without support for those well-known types, and then either fork protobuf.js, patch it at runtime, or wait for it to be fixed upstream. |
Hey @isherman apologies for the really late reply here. It'd be awesome if you could poke around at this. Per the protobuf.js PRs, I think that shouldn't be a problem, because ts-proto implements its own direct support for the proto3 JSON protocol. I.e. here is where we generate https://github.com/stephenh/ts-proto/blob/master/src/main.ts#L759 We don't handle encoding/encoding Durations yet. So, in theory, a JSON client would just be some glue that takes the |
To make it really generic, we could also:
interface Rpc {
- request(service: string, method: string, data: Uint8Array): Promise<Uint8Array>;
+ request<Req extends Codec, Res extends Codec>(service: string, method: string, req: Req): Promise<Res>;
} where // This is the interface that's already implemented by all types in ts-proto,
// i only gave it a name
interface Codec<T> {
encode(message: T, writer: Writer = Writer.create()): Writer;
decode(input: Uint8Array | Reader, length?: number): T;
fromJSON(object: any): T;
fromPartial(object: DeepPartial<T>): T;
toJSON(message: T): unknown;
}
const implWithUint8Array: Rpc = {
request<Req, Res>(service: string, method: string, req: Req): Promise<Res> {
const data = Req.encode(req).finish();
// Do whatever you used to do before, e.g. gRPC
const wireResponse = ...;
return Res.decode(new Reader(wireResponse))
}
}
const implWithJSON: Rpc = {
request<Req, Res>(service: string, method: string, req: Req): Promise<Res> {
const data = Req.toJSON(req);
// Do whatever you used to do before, e.g. gRPC with JSON
const wireResponse = ...;
return Res.fromJSON(wireResponse);
}
} Advantages:
|
@amaurymartiny Agree. In my case, I also need the Res codec too. interface Rpc {
- request(service: string, method: string, data: Uint8Array): Promise<Uint8Array>;
+ request<Req extends Codec, Res extends Codec>(
+ service: string,
+ method: string,
+ data: Uint8Array,
+ req: Req,
+ res: Res
+ ): Promise<Uint8Array>;
} |
Thanks for picking this up @disjukr, @amaurymartiny, @stephenh. Your approach looks more general, an improvement for sure. One tradeoff is it's a breaking change. |
any moving forward for this issue? |
We have a use case for sending JSON data via the Additionally, and more importantly, the protobuf.js file that the binary serialization/deserialization relies upon uses the Would you be open to a contribution which implements something along the lines of either #106 or this suggestion? I imagine this would be gated behind a flag like |
Attempted an implementation of a generic client here |
Correct me if I'm wrong, but it seems like the auto-generated client impl is hard-coded to use protobufs over the wire:
Would it be possible to generate a parallel client impl that uses JSON over the wire instead?
The text was updated successfully, but these errors were encountered: