stephenh / ts-proto

An idiomatic protobuf generator for TypeScript
Apache License 2.0
2.13k stars 345 forks source link

support for custom options in service / rpc #570

Open NimrodAvni opened 2 years ago

NimrodAvni commented 2 years ago

I think a great feature to have is for custom options from the proto definition to be present in the generated code something similar to protobuf-ts is doing. for example when generating a generic service definition from

service MyService {
    rpc MyMethod (MyMethodRequest) returns (MyMethodResponse) {
        option (google.api.http) = {
            get: "/v1/my-method"
        };
    };
}

the resulting code would be something like

export const MyServiceDefinition = {
  name: "MyService",
  fullName: "com.something.v1.MyService",
  methods: {
    myMethod: {
      name: "MyMethod",
      requestType: MyMethodRequest,
      requestStream: false,
      responseType: MyMethodResponse,
      responseStream: false,
      options: {
         "google.api.http": {
            get:"/v1/my-method""
         }
      },
    }
}

i know that options is reserved for more well-defined options like idempotencyLevel, so it might be better to have it in a separate options object like unknownOptions or something like that

stephenh commented 2 years ago

Hi @NimrodAvni ; @Vilsol just landed a PR that outputs options, if you use the outputSchema flag:

https://github.com/stephenh/ts-proto/pull/437

It looks like you want the option output as part of the service definitions, and maybe just that, not the entire outputSchema output?

If so, that makes sense; that's probably something that could be built on top of @Vilsol 's work?

Happy to have a PR for it if you want to work on it. Thanks!

NimrodAvni commented 2 years ago

thanks for the answear! I didn't know the outputSchema option does that thats actually exactly what im looking for! unfortunately when i enabled it the outputed schema does not compile for me when i run tsc

image

this error with the & Record<unique symbol | unique symbol | "flatMap" | "flat" | "at", never> comes up on a lot of fields in the FileDescriptorProto1.fromPartial method in all generated proto files i dont know if my tsconfig is configure badly or something because the type signature seems to match and not contain the & Record<...> type, but building succeeds without the outputschema option

am i missing something? anything needs to be configured differently @stephenh @Vilsol ?

Vilsol commented 2 years ago

@NimrodAvni Try using ES2018 and enabling strictNullChecks

You can also try disabling exact types: --ts_proto_opt=useExactTypes=false

Vilsol commented 2 years ago

Actually, this seems like a bigger issue than I first thought. It looks like if you import types for NodeJS > 14, the Exact completely breaks.

Our 2 options for solving this is either figure out how to support Exact on >14, or not using Exact types in ts-proto-descriptors @stephenh

NimrodAvni commented 2 years ago

hi @Vilsol @stephenh ,is there any update on this?

stephenh commented 2 years ago

@NimrodAvni ah sorry, not really; @Vilsol I wasn't really following the issue with Exact; right now the ts-proto test suite runs on both node 14.x and 16.x, so I'm surprised it does work on > 14?

As a disclaimer, that Exact is home-grown/adhoc, so despite being surprised about it not working on node > 14, I am also not surprised it's not perfect.

If the quickest win unblock is to build ts-proto-types w/o Exact types, I'm good with that, just disclaimer I'd +1 the PR / merge the release, but am not 100% following along (due to lack of available time, sorry).

stephenh commented 2 years ago

@Vilsol FWIW while bumping to the latest TypeScript in #603, I saw some Exact weirdness in ts-proto-descriptors, which somewhat magically went away when I bumped ts-proto-descriptors to the latest TypeScript.

Does this new ts-proto-descriptors 1.7.0 fix the issue you're seeing here?

Happy to turn off exact types in ts-proto-descriptors all together, but just curious...

Also fwiw I was seeing "type is any" / "_unknownFields" can't be used to access message issues on these lines:

            for (const key of Object.keys(message['_unknownFields'])) {

So had to disable strict checks in the protos/tsconfig.json for now; seems like that might be an issues for the unknownFields output in general for newer versions of TypeScript? Like maybe we need to throw in an "as any" or add a hasUnkownFields(mesage) type guard to the output...

paralin commented 2 years ago

@stephenh It was just complaining that message doesn't have a _unknownFields property.

No need to disable strict checks; fixed it this way: https://github.com/stephenh/ts-proto/pull/603/files#diff-3afb0e5a47105f8dece6c028029154c935e40325133f8b41846bbb348ed5adeaR365

stephenh commented 2 years ago

@paralin thanks for getting that in! Yep, I only disabled strict checks b/c I was rushing to get ts-proto-descriptors released for your PR. With your unknownFields fix merged, I just bumped ts-proto-descriptors version of ts-proto and put the strict: true back. Thanks!