Skip to content
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

Open
alecthomas opened this issue Sep 29, 2019 · 9 comments
Open

Generate a JSON client #7

alecthomas opened this issue Sep 29, 2019 · 9 comments
Labels
help wanted Extra attention is needed

Comments

@alecthomas
Copy link

Correct me if I'm wrong, but it seems like the auto-generated client impl is hard-coded to use protobufs over the wire:

  geoIP(request: GeoIPReq): Promise<GeoIPResp> {
    const data = GeoIPReq.encode(request).finish();
    const promise = this.rpc.request("service.Service", "geoIP", data);
    return promise.then(data => GeoIPResp.decode(new Reader(data)));
  }

Would it be possible to generate a parallel client impl that uses JSON over the wire instead?

@alecthomas
Copy link
Author

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.

@stephenh
Copy link
Owner

@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 generateRegularRpc function that could be the first place to add some conditional "if output type" type logic:

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.

@stephenh stephenh changed the title Generate a JSON client? Generate a JSON client Feb 17, 2020
@stephenh stephenh added the help wanted Extra attention is needed label May 3, 2020
@isherman
Copy link

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 Timestamp and Duration.

And google's JS protobuf library has no support at all: protocolbuffers/protobuf-javascript#95

Do you see any way forward?

@isherman
Copy link

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.

@stephenh
Copy link
Owner

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 Message.fromJSON methods and handle timestamps:

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 service interfaces and makes a <Service>JsonClient that uses ts-proto's existing fromJSON/toJSON methods to serde the messages.

@amaury1093
Copy link

amaury1093 commented Dec 3, 2020

To make it really generic, we could also:

  1. Change the signature of the request function to take
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 Codec is just:

// 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;
}
  1. Then, it would be the user's responsability, when implementing the Rpc interface, to decide which wire format to use:
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:

  • no need for an additional protoc option
  • allow users to use Uint8Array, JSON for wire transfer... or do other more fancy stuff.

@disjukr
Copy link

disjukr commented Dec 15, 2020

@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>;
}

@isherman
Copy link

Thanks for picking this up @disjukr, @amaurymartiny, @stephenh.
I lost momentum on #106 when the motivation for JSON serialization became less clear in my application. Will close that now.

Your approach looks more general, an improvement for sure. One tradeoff is it's a breaking change.
But I'm no longer a stakeholder here, so I defer to the maintainer!

@vtolstov
Copy link

any moving forward for this issue?

zfy0701 added a commit to sentioxyz/ts-proto that referenced this issue Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants