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

feat(microservices): add package definition option to server-grpc #10530

Merged

Conversation

krugi
Copy link
Contributor

@krugi krugi commented Nov 7, 2022

PR #8465 added support for packageDefinition for ClientGrpc, this PR adds it to ServerGrpc.

  • Export common packageDefinition logic to getGrpcPackageDefinition and used it in both gRPC Server & Client, for consistency
  • This change means that GrpcOptions['options']['protoPath'] is now optional - getGrpcPackageDefinition verifies that either protoPath or PackageDefinition exist
  • Also, changed the error thrown from ServerGrpc.loadProto: it was Error, now InvalidProtoDefinitionException, as it is in ClientGrpc
  • Add a loadProto test for ServerGrpc, similar to the one that exists for ClientGrpc

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Currently, it's impossible to create a gRPC microservice by providing a packageDefinition, which means statically generated code can't be used — users must provide *.proto files. This situation isn't suitable for all (e.g. if your organization holds a dedicated repo for Protobufs, rather than storing them in the nestjs-based API repo they're developing, which is quite common).

Issue Number: N/A

What is the new behavior?

It's possible to create a gRPC microservice by providing packageDefinition.

This change is inspired by #8465, which allowed packageDefinition to be provided when creating a gRPC client. I think that the fact that the author kept GrpcOptions['options']['protoPath'] as a required field is confusing and inconsistent - even when providing packageDefinition, one still needs to provide protoPath which will simply be ignored. In this PR I suggest that protoPath will be optional, and when initiating the gRPC server we'll make sure that either protoPath or packageDefinition are supplied. I'd like to hear your thoughts on that.

Does this PR introduce a breaking change?

  • Yes
  • No
  • Kinda :)

I'm not sure if changing protoPath to optional is considered a breaking change... either way, the documentation should be updated as well, so users will know it's not protoPath that's required, but either protoPath or packageDefinition. The docs are in a separate repo, right?

Other information

I'm open for discussion and willing to move around code (for example, I'm not sure that the getGrpcPackageDefinition validation should happen in loadProto, maybe in the constructor?).

Thanks!

PR nestjs#8465 added support for packageDefinition for ClientGrpc, this commit adds it to ServerGrpc.
Exported common packageDefinition logic to getGrpcPackageDefinition and used it in both gRPC Server & Client, for consistency.
This change means that protoPath is now optional - getGrpcPackageDefinition verifies that either protoPath or PackageDefinition exist.
Also, fixed the error thrown from ServerGrpc.loadProto: was Error, now InvalidProtoDefinitionException, as it is in ClientGrpc
@coveralls
Copy link

Pull Request Test Coverage Report for Build a5d9892f-7f29-42af-a880-6ccde79c9605

  • 23 of 24 (95.83%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 93.924%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-grpc.ts 3 4 75.0%
Totals Coverage Status
Change from base Build db07b797-b226-4c6c-9b82-22af4fb43cb5: 0.1%
Covered Lines: 6168
Relevant Lines: 6567

💛 - Coveralls

@gigi
Copy link

gigi commented Oct 16, 2023

Hello!
Is there any chance to merge this?)

@jtimmons
Copy link
Contributor

jtimmons commented Nov 1, 2023

bumping this since it could help solve some major difficulties that exist when attempting to share controllers for gRPC services (documented in #12560)! In fact, if this accepted multiple PackageDefinitions rather than just a single one then I think it would entirely solve that usecase because then each external library could be completely in control of important proto-loading options like keepCase rather than relying on the global GrpcClientOptions values

I'd be happy to take a stab at contributing that if/when this gets merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants