Conversation
Signed-off-by: NorthBlue333 <north333@free.fr>
NorthBlue333
force-pushed
the
streamings-and-tls
branch
4 times, most recently
from
dafaf2b to
2ba4d42
Compare
NorthBlue333
force-pushed
the
streamings-and-tls
branch
3 times, most recently
from
885e7d4 to
3dbf936
Compare
There was a problem hiding this comment.
Thanks for sharing your contributions, @NorthBlue333!
I've skimmed through and added my initial comments; These are not necessarily requests for changes, but more to initiate the discussion and better understand the changes.
I'll leave it to @raymondfeng to spearhead the review.
src/decorators/grpc.decorator.ts
Outdated
| const spec = getLoopbackMetadataFromMethodDescriptor( | ||
| findMethodInFileDescriptor( | ||
| proto, | ||
| target.constructor.name.replace( | ||
| options?.controllerNameRegex ?? new RegExp(/(Ctrl|Controller)$/, 'g'), | ||
| '', | ||
| ), | ||
| `${methodName[0].toUpperCase()}${ | ||
| methodName.length ? methodName.slice(1) : '' | ||
| }`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Would it be possible to enable explicit declaration of the gRPC method name via the decorator instead (i.e. Like how it was before)?
I understand the rationale for inferring this automatically, but IMO doing things auto-magically has its pitfalls of being more difficult to debug. It's also not as aligned with the "LoopBack 4 approach" of giving developers control and flexibility.
There was a problem hiding this comment.
Well yes. I had done this because of limitations of the compiler.
I am currently trying to do something else, more explicit.
There was a problem hiding this comment.
I have modified the decorator. It now takes an object as argument, with all the necessary properties. I have also provided a helper function which takes the fileDescriptor and the method definition.
Sadly, I don't see any other way to do this because of how the typescript files are generated from the proto files.
| async clientStreamTest( | ||
| request: Observable<TestRequest>, | ||
| ): Promise<TestResponse> { | ||
| const names: string[] = []; | ||
| let error; | ||
| const observer: PartialObserver<TestRequest> = { | ||
| next: (value) => names.push(value.name), | ||
| error: (err) => (error = err), | ||
| }; | ||
| request.subscribe(observer); | ||
| await lastValueFrom(request); | ||
| if (error) { | ||
| throw error; | ||
| } | ||
| return { | ||
| message: 'Test ' + request.name, | ||
| message: names.join(' '), | ||
| }; |
There was a problem hiding this comment.
I don't have an in-depth knowledge on rxjs; Is there a benefit over the previous "plain functions" for gRPC?
There was a problem hiding this comment.
I don't think it adds anything more than encapsulation (and I think it is less efficient). I have updated my PR, using implementations of "plain functions" for @gRPC/grpc-js
NorthBlue333
force-pushed
the
streamings-and-tls
branch
2 times, most recently
from
5c14227 to
eb8fb44
Compare
Signed-off-by: NorthBlue333 <north333@free.fr>
NorthBlue333
force-pushed
the
streamings-and-tls
branch
2 times, most recently
from
cd594c6 to
8a5f78b
Compare
Signed-off-by: NorthBlue333 <north333@free.fr>
NorthBlue333
force-pushed
the
streamings-and-tls
branch
from
8a5f78b to
aa116bc
Compare
Signed-off-by: NorthBlue333 <north333@free.fr>
Signed-off-by: NorthBlue333 <north333@free.fr>
Signed-off-by: NorthBlue333 <north333@free.fr>
Signed-off-by: NorthBlue333 <north333@free.fr>
Signed-off-by: NorthBlue333 <north333@free.fr>
2 participants
Description
While working on a work project, I had to implement gRPC in Loopback 4. After implementing streams and TLS, I thought it might be useful to others. I am sorry for not following all the rules to contribute to Loopback 4, but I am trying ! I am of course open to suggestions.
This PR adds :
Also, I have updated the protoc binaries, and switched to proto-ts plugin to generate Typescript interfaces.
I have also added tests to cover all changes, but editing your hosts file is needed to run both TLS and mTLS tests.
Finally, I tried to add docs for all these implementations.
Note: most packages have been updated too.
Related issues
Checklist
guide