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

Configure GRPC client method case #9047

Closed
1 task done
TomChv opened this issue Jan 27, 2022 · 4 comments
Closed
1 task done

Configure GRPC client method case #9047

TomChv opened this issue Jan 27, 2022 · 4 comments
Labels
needs triage This issue has not been looked into type: enhancement 🐺

Comments

@TomChv
Copy link

TomChv commented Jan 27, 2022

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

My request is not a problem but more a story of UX.
When we want to build a gRPC microservice, we write one or more .proto file that will define your different services and message.

Here's a simplistic example that follow the google API naming convention.

syntax = "proto3";

import "google/protobuf/empty.proto";

package example.task;

service TaskService {
    rpc ListTasks (google.protobuf.Empty) returns (ListTasksResponse) {}
    rpc CreateTask (CreateTaskRequest) returns (Task) {}
}

message Task {
    int32 id = 1;
    string name = 2;
}

message ListTaskResponse {
    repeated Task tasks = 1;
}


message CreateTaskRequest {
    string name = 1;
}

The current issue is that NestJS gRPC client only define lowercase methods which is really annoying because even if you use a tool to generate your type like ts-protoc-gen, your client interface will be wrong.

And it required to manually define an interface that it's just a mirror of generated method but with a lowercase.

To follow my example, we will have to do this

export interface TaskService {
    listTasks(): Observable<ListTasksResponse>
    createTask(task: CreateTaskRequest): Observable<Task>
}

// In the client
export class TaskClientService implements OnModuleInit {
    private taskService: TaskService
    
    // Inject client...

    onModuleInit() {
	this.taskService = this.client.getService<TaskService>('TaskService');
     }

    listTasks() {
       return this.taskService.listTasks(); // Will call ListTasks RPC method
    }
}

The necessity to define a second source of truth is not relevant and can become painful for huge project.

Describe the solution you'd like

I would like to simply add an optional parameter to the getService method from the gRPC Client to optionally keep the method in PascalCase.

Click to jump to the actual piece of code:

I would like to improve it to something like this

  interface IGetServiceOptions {
      // Keep given method name case 
      keepCase: boolean;
  }


  public getService<T extends {}>(name: string, opts?: IGetServiceOptions): T {
    const grpcClient = this.createClientByServiceName(name);
    const clientRef = this.getClient(name);
    if (!clientRef) {
      throw new InvalidGrpcServiceException();
    }

    const protoMethods = Object.keys(clientRef[name].prototype);
    const grpcService = {} as T;

    // I'm sure we can do this in a cleaner way, that's an example.
    const getKey(k: string): string {
         if (opts.keepCase) {
             return k;
         }
         return m[0].toLowerCase() + m.slice(1, m.length);
    }

    protoMethods.forEach(m => {
      const key = getKey(m);
      grpcService[key] = this.createServiceMethod(grpcClient, m);
    });
    return grpcService;
  }

This way, if I take back my example, we will be able to do

// No more interface is required
// In the client
export class TaskClientService implements OnModuleInit {
    private taskService: TaskService
    
    // Inject client...

    onModuleInit() {
        // Use interface generated by `ts-protoc-gen`
	this.taskService = this.client.getService<TaskService>('TaskService');
     }

    listTasks() {
       return this.taskService.ListTasks(); // Will call ListTasks RPC method
    }
}

Teachability, documentation, adoption, migration strategy

There is nothing really special to about the release of that new feature.
Just a couple of word in the documentation and the changelog should do the job.

Indeed, it would be amazing to improve the current NestJS gRPC sample example or maybe create another one more complex.
I would be happy to work on it by the way 🚀

What is the motivation / use case for changing the behavior?

It's a pain to maintain multiple source of truth, with this option, it will helps developers to adopt NestJS gRPC!

@TomChv TomChv added needs triage This issue has not been looked into type: enhancement 🐺 labels Jan 27, 2022
@TomChv
Copy link
Author

TomChv commented Jan 27, 2022

Indeed, I'm volunteer to create a PR that apply those changes, but I wanted first to write a proposal in order to be sure that it was a good idea.

@TomChv
Copy link
Author

TomChv commented Jan 28, 2022

I saw that after digging into gRPC client, proto loads method both in lowercase and in uppercase, you can test with the following line

    // nest/packages/microservices/client/client-grpc.ts L.58
    protoMethods.forEach(m => {
      const key = key[0].toLowerCase() + key.slice(1, key.length);
      console.log(key, m);
      grpcService[key] = this.createServiceMethod(grpcClient, m);
    });
# Output for GRPC transport sum.proto sum2.proto
sum Sum
sum sum
sumStream SumStream
sumStream sumStream
sumStreamPass SumStreamPass
sumStreamPass sumStreamPass
Sum Sum
sum sum
SumStream SumStream
sumStream sumStream
SumStreamPass SumStreamPass
sumStreamPass sumStreamPass

Since we already looping through bot cases, I think we should just stop force lowercase, this way, user can choose himself between lower or upper case ;)

TomChv pushed a commit to TomChv/nest that referenced this issue Jan 29, 2022
RPC methods in protobuf are commonly declare in PascalCase, but they were generated lowercase
by ClientGrpcProxy.getService method.
Protobuf and gRPC have tools to generate interface and message in Typescript directly from
protobuf. It's a powerful feature that's helping to maintain only one source of truth.
But with that lowercase method generation, developers were forced to define their own interface
which can be painful to maintain.
This commit fix that issue by generating methods both in lowercase and in uppercase.

Solves nestjs#9047

Signed-off-by: Vasek - Tom C <[email protected]>
@kamilmysliwiec
Copy link
Member

Fixed here #9052

@ssilve1989
Copy link
Contributor

I am confused by this request/change. It is the responsibility of the protogen tool to generate the interfaces to fit the consumers need. Most protoc plugins or generators offer options to generate typescript service interfaces with PascalCase or the standard default, camelCase. The example provided here ts-protoc-gen by default generates them in camelCase which matches what the Nest grpc client was doing does it not?

If Nest wanted to bundle their own grpc service typescript generation into the CLI or something that would be a different story.

It's certainly not a big deal to let Nest have this option, I just don't think its Nest's responsibility to do so when the language standard convention is camelCase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into type: enhancement 🐺
Projects
None yet
Development

No branches or pull requests

3 participants