ts-proto icon indicating copy to clipboard operation
ts-proto copied to clipboard

Are generated services supposed to required complete messages (no DeepPartial)?

Open jschaf opened this issue 4 years ago • 1 comments
trafficstars

First off, I love this library. It's by far the nicest protobuf library for TypeScript. I noticed that generated client implementations require a complete message for all RPC method. Two questions:

  • Is requiring a complete request a design choice or an implementation detail? I noticed the that grpc-web generated code uses DeepPartial by default. I'd prefer DeepPartial as it doesn't require me to initialize every field.
  • What's the recommended way to pass a partial object to the client impl? I'm guessing I need to enable outputPartialMethods and use fromPartial.

I'm invoking the library like:

protoc --plugin=./node_modules/.bin/protoc-gen-ts_proto \
      --ts_proto_opt=env=browser \
      --ts_proto_opt=exportCommonSymbols=false \
	  --ts_proto_opt=outputJsonMethods=false \
	  --ts_proto_opt=outputPartialMethods=false \
	  --ts_proto_out=./proto/ \
	  --proto_path ../api \
	  --proto_path ../../third_party/protobuf \
	  product.proto

That generates this code:

export interface ProductSvc {
  CreateProduct(request: CreateProductRequest): Promise<Product>;
}

export class ProductSvcClientImpl implements ProductSvc {
  private readonly rpc: Rpc;
  constructor(rpc: Rpc) {
    this.rpc = rpc;
  }

  CreateProduct(request: CreateProductRequest): Promise<Product> {
    const data = CreateProductRequest.encode(request).finish();
    const promise = this.rpc.request(
      "ProductSvc",
      "CreateProduct",
      data
    );
    return promise.then((data) => Product.decode(new Reader(data)));
  }
}

jschaf avatar Apr 01 '21 23:04 jschaf

Hey! Thanks, glad you're enjoying ts-proto!

complete request a design choice or an implementation detail

Yeah, it's a design choice on my part insofar as wanting the type-safety of "the programmer really knows what they're sending over the wire", vs. having defaults show up without their intent.

I forget why exactly I didn't do that for the grpc-web side, although frankly ~80% was that I had a paying client that specifically asked for it. :-D

I think, either remembering my rationale, or making one up that makes sense, fields in ts-proto types are primarily non-nullable to make business logic that reads them easier.

I.e. instead of if (message.field !== undefined && message.field !== 0) or something like that, the protobuf spec just says "thou shalt always see 0", at least on the reader side, so that's why I want field to be field: number instead of field?: number on Message itself.

I guess, that said, getting to the rationale about grpc-web, that is a writer-based scenario, i.e. we're passing methods into a service call, and does it really matter if we pass 0 or undefined and the internal serialization just does the right thing anyway?

Probably not, so I'd be fine changing the non-grpc-web methods over to take DeepPartials as well, for consistency and ergonomics.

Are you interesting in submitting a PR?

I need to enable outputPartialMethods and use fromPartial

Yes, that's what you'd do for now.

stephenh avatar Apr 02 '21 19:04 stephenh

Are you interesting in submitting a PR?

Heh, I ended up rolling my own proto to TypeScript compiler. Here's the interesting design decisions:

  • Messages are plain objects. I regret this choice now. I wish I had done classes because that let's you do lazy decoding (with getters) and other tricks. The one upside of objects is that you could do deserialization in a web-worker (but I think lazy decoding is a more promising method).
  • We use resource-oriented design, so I made the string name fields strongly typed.
  • Used Twirp's transport using POST HTTP. I'd use buf.build if it was around when I started.
  • Support message-level remapping, so a field like google.type.Money gets converted to a full TypeScript Money class with useful methods.

jschaf avatar Aug 22 '23 17:08 jschaf