grpc-ecosystem / protoc-gen-grpc-gateway-ts

protoc-gen-grpc-gateway-ts is a Typescript client generator for the grpc-gateway project. It generates idiomatic Typescript clients that connect the web frontend and golang backend fronted by grpc-gateway.
Apache License 2.0
142 stars 51 forks source link

renderURL does not use lowerCamelCase field names #5

Closed atreya2011 closed 3 years ago

atreya2011 commented 3 years ago

Given a proto file with the following annotation and fieldname for GetUserRequest:

service UserService {
  rpc GetUser(GetUserRequest) returns (GetUserResponse) {
    option (google.api.http) = {
      get: "/api/v1/user/{user_id}"
    };
  }
}

message GetUserRequest {
  string user_id = 1;
}

message GetUserResponse {
  User user = 1;
}

message User {
  string id   = 1;
  string name = 2;
}

generates the following TypeScript:

/*
* This file is a generated Typescript file for GRPC Gateway, DO NOT MODIFY
*/

export type GetUserRequest = {
  userId?: string
}

export type GetUserResponse = {
  user?: User
}

export type User = {
  id?: string
  name?: string
}

export class UserService {
  static GetUser(req: GetUserRequest, initReq?: fm.InitReq): Promise<GetUserResponse> {
    return fm.fetchReq<GetUserRequest, GetUserResponse>(`/api/v1/user/${req["user_id"]}`, {...initReq, method: "GET"})
  }
}

Which throws a type error because of /api/v1/user/${req["user_id"]} as user_id does not exist in the generated type definition for GetUserRequest

I have fixed this locally. Is it ok to create a PR?

lyonlai commented 3 years ago

hmm. there are supposed to be tests covering it but not sure how it failed, which concerns me. I'll take a look at this. Thanks for reporting. will let you know soon @atreya2011

atreya2011 commented 3 years ago

Likewise, thank you for making this excellent plugin! 🙇🏼 I have fixed this locally and make a PR on this 🙏🏼

https://github.com/atreya2011/protoc-gen-grpc-gateway-ts/commit/b8d327e8721d13c3eed2814a99c9df2d89f73250

lyonlai commented 3 years ago

Thanks for being so passionate about this project. The fix itself is easy but I would like to adjust the test to catch this problem as well. That's why I prefer to take a look and let you know soon @atreya2011

I see the support for HTTP Patch is in the PR as well. Much appreciate the contribution but I think it would be better in it's own PR.

atreya2011 commented 3 years ago

Understood. Thank you very much! I will make a different PR for the HTTP Patch fix.

And this is just the plugin I was looking for because I have the same pain points that you mentioned in this blog post 👇🏼 https://cashapp.github.io/2021-03-26/protoc-gen-grpc-gateway-ts-clean-idiomatic-typescript-for-grpc-gateway