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

GRPC: Fix credentials and introduce arguments #4607

Closed
wants to merge 4 commits into from

Conversation

sjkummer
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] 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?

GRPC credentials were completly ignored on client side. It was no more possible to establish a secure SSL GRPC-connection. Further, it was no more possible to pass GRPC-arguments

Issue Number: #4597

What is the new behavior?

GRPC-credentials work again (as they have in Nestjs6). Additionaly, an argument parameter was added to the GRPC options, to pass any of the arguments defined in https://grpc.github.io/grpc/core/group__grpc__arg__keys.html

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@coveralls
Copy link

coveralls commented Apr 16, 2020

Pull Request Test Coverage Report for Build 23046

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.632%

Totals Coverage Status
Change from base Build 20728: 0.0%
Covered Lines: 4689
Relevant Lines: 4955

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

What do you think about copying/mimicking options from this file:
https://github.com/grpc/grpc-node/blob/master/packages/grpc-js/src/channel-options.ts
instead of introducing a new loosely typed arguments property? Another solution would be to add [key: string]: any; to the GrpcOptions['options']

...maxMessageLengthOptions,
...keepaliveOptions,
};

const credentials =
options.credentials || grpcPackage.credentials.createInsecure();
this.options.credentials || grpcPackage.credentials.createInsecure();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this fix is quite important, I'll push it as a separate commit

@sjkummer
Copy link
Contributor Author

sjkummer commented Apr 16, 2020

Hm, what about exporting the internally used grpc package? This will:

  1. Allow typed GRPC-options for credentials, arguments and so on, without code duplication
  2. This will avoid Type Errors, which occur if the Application does not use the exactly same GRPC version to create the credentials as Nestjs uses internally (this actually only works because npm dedups node_modules, see Generated client fails in Node on latest version grpc/grpc#10786 )

@kamilmysliwiec
Copy link
Member

Although, I'd love to do it, grpc is an optional package and re-exporting types that come from the optional library will lead to compilation errors if skipLibCheck (in tsconfig) is set to true (which is the case for both starter project and CLI)

@sjkummer
Copy link
Contributor Author

@kamilmysliwiec Finally, I've fixed the tests 😅

@kamilmysliwiec
Copy link
Member

Thank you!

@kamilmysliwiec
Copy link
Member

🤦 Just FYI: I've clicked "Close" by accident, but your PR has already been merged! I can't reopen now since the "These commits are already merged" shows up

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.

3 participants