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 ourterms of serviceand privacy statement.We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gRPC proto-loader options are global to the server rather than per-package #12560

Open
3 of 15 tasks
jtimmonsopened this issue Oct 12, 2023 · 1 comment
Open
3 of 15 tasks
Labels
needs triage This issue has not been looked into

Comments

@jtimmons
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

We currently use a globalGrpcOptionsobject to create a gRPC server which includes proto-loader configuration in theGrpcOptions.options.loaderfield. This is a departure from howgrpc-jsworks in which you would normally:

  1. Create aPackageDefinitionfrom the proto files by loading it viaproto-loader
  2. Load thatPackageDefinitioninto aProtoDescriptorusinggrpc-jsand add that into the server
  3. Repeat for any additional service implementations that you have

By attaching the proto-loader configuration to theserverinstead of thepackage definitionwe lose some flexibility in how people configure their implementation logic. Particularly, this becomes an issue when attempting to use shared modules for common logic (eg.grpc-health-check,reflection-api)

Because the common logic is being loaded into someone else's configuration it has no control over settings which may be important to the implementation. Specifically: defaults for missing fields can be different based on that configuration, while thekeepCaseoption can change entire field names

Minimum reproduction code

https://gitlab /jtimmons/nestjs-grpc-reflection-module/-/commits/temp/remove-req-res-conversion

Steps to reproduce

The linked reproduction is to a library that I maintain (nestjs-grpc-reflection-module) where I've had to work around this behavior. The linked branch disables that workaround behavior so that you can demonstrate it in the repo's sample server.

To reproduce:

  1. Startup sample server withnpm i && npm run start:dev
  2. TurnkeepCaseon/off in the sample server'sgrpc-client options
  3. The sample server will not work whenkeepCaseis set totrue

The reflection module code is written to expect the defaultkeepCaseoption offalseand so, with the workaround logic disabled, will break whenkeepCaseis set totrue.This is because it will receive messages with field names such aslist_serviceswhen the code itself is trying to access that field as the camel-cased name oflistServices

Expected behavior

I would expect that we would be able to control the proto-loader configuration per module somehow

Package

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

10.2.6

Packages versions

[System Information]
OS Version: macOS
NodeJS Version: v18.17.1
NPM Version: 9.6.7

[Nest CLI]
Nest CLI Version: 10.1.18

[Nest Platform Information]
schematics version: 10.0.2
testing version: 10.2.6
config version: 3.1.1
cli version: 10.1.18

Node.js version

v20.8.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

PR#10530doesn't directly fix this but may offer a good path forward? If a module could add a fullPackageDefinitionto the list of services to add to a gRPC server then it could solve the problem somewhat

@kamilmysliwiec
Copy link
Member

#10530has just been merged

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
Projects
None yet
Development

No branches or pull requests

2 participants