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.

Sign up for GitHub

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

Jump to bottom

fix(microservices): generate service method in lower and uppercase #9052

Merged
merged 1 commit into from
Feb 14, 2022
Merged

fix(microservices): generate service method in lower and uppercase #9052

merged 1 commit into from
Feb 14, 2022

Conversation

TomChv
Copy link

@TomChv TomChv commented Jan 29, 2022

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?

Issue Number: #9047

What is the new behavior?

gRPCClientProxy.GetService now generates service methods both in uppercase and lowercase instead of forcing it to lowercase.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@TomChv
fix(microservices): generate service method in lower and uppercase
23170cf 
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]>
@coveralls
Copy link

Pull Request Test Coverage Report for Build 321d29c4-0ed5-4e62-8a15-e32308c1488f

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 94.146%
Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/client/client-grpc.ts 0 1 0.0%
Totals Coverage Status
Change from base Build a61c247b-30c1-4808-b18a-03c880125da7: 0.02%
Covered Lines: 5693
Relevant Lines: 6047
💛 - Coveralls

@kamilmysliwiec kamilmysliwiec merged commit 7f170d2 into nestjs:master Feb 14, 2022
@kamilmysliwiec kamilmysliwiec mentioned this pull request Feb 14, 2022
Closed
1 task
@TomChv
Copy link
Author

TomChv commented Feb 14, 2022

Thank you @kamilmysliwiec 🚀

Just a quick question, why I do not appears in NestJS contributors?

@TomChv TomChv deleted the fix/gprc-service-method-case branch February 14, 2022 10:37
@pachuka
Copy link

pachuka commented Apr 8, 2022

@kamilmysliwiec @TomChv - ran into an interesting issue with our NestJS microservices after upgrading from 8.2.6 to 8.3 and narrowed it down to this change. Not sure I'd necessarily call it a bug but this did end up being a breaking change for us. One of our client api functions is named "generateCOIForms" where COI is an acronym, on version 8.2.6 the function name generated would match and also be generateCOIForms after this change we're seeing GenerateCOIForms and generateCoiForms (note the OI getting lowercased). Just wanted to call it out in case anyone else runs into it.

Thanks!

@TomChv
Copy link
Author

TomChv commented May 18, 2022

@pachuka

According to changes in the code, there is no transformation of the name in it.
Have you done a git bisect to verify that it comes from this commit?

@Drew-Kimberly
Copy link

@pachuka did you dig into this any deeper? We have the same issue where our grpc method getXYZId is generating a getXyzId and GetXYZId method (whilst the TS interface has getXYZId).

@pachuka
Copy link

pachuka commented Nov 2, 2022

@Drew-Kimberly - My bad I completely forgot about this, things have been busy :). I'm fairly certain the issue was introduced with this change (you can try by downgrading to previous and it should work as expected). I may have time this Friday to take a look and make a reproduction. Will note that down in my Todos. Sorry for the delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
scope: microservices type: enhancement 🐺
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TomChv @coveralls @pachuka @Drew-Kimberly @kamilmysliwiec