[DO-NOT-MERGE] spike - gRPC support #571
Conversation
bajtos
changed the title
Add an initial version of gRPC support, using API/protocol-first approach.
| // tslint:disable-next-line:no-any | ||
| const greeter: any = {}; | ||
|
|
||
| Object.keys(client.__proto__).forEach(method => { |
There was a problem hiding this comment.
Use Object. getPrototypeOf(client) instead?
| // TODO(bajtos) serviceSpec needs to be typed | ||
| export function service(serviceSpec: any) { | ||
| return function(controllerCtor: Function) { | ||
| (controllerCtor as any).grpcService = serviceSpec; |
There was a problem hiding this comment.
Is it a good practice to mutate the class directly instead of Reflector?
There was a problem hiding this comment.
I see two issues with Reflector:
- It relies on having a single
reflect-metadatapackage in node_modules tree, which cannot be reliably achieved. - It's difficult to use from JavaScript codebase.
Creating a static property on the constructor class solves both issues and it's the same approach we are already using for Model.definition in our repository package. I think we should eventually use this approach for storing OpenAPI spec too.
Thoughts?
There was a problem hiding this comment.
+1, for the first reason alone; if others are making use of reflect-metadata, which is entirely likely, it could impact us.
packages/core/src/application.ts
Outdated
| methodName: string, | ||
| rootContext: Context, | ||
| ) { | ||
| // TODO(bajtos) support metadata? |
There was a problem hiding this comment.
request.metadata contains gRPC metadata from the client call, like http headers.
packages/core/src/application.ts
Outdated
| debug('gRPC invoke %s.%s(%j)', controllerName, methodName, request); | ||
|
|
||
| // FIXME(bajtos) Create new per-request child context | ||
| // FIXME(bajtos) Use Sequence to allow custom request processing |
packages/core/src/application.ts
Outdated
| return function(request: any, callback: any) { | ||
| debug('gRPC invoke %s.%s(%j)', controllerName, methodName, request); | ||
|
|
||
| // FIXME(bajtos) Create new per-request child context |
There was a problem hiding this comment.
+1. The request.metadata can be bound to the child context.
| } | ||
|
|
||
| it('provides gRPC interface for decorated controllers', async () => { | ||
| @service(helloService.Greeter.service) |
There was a problem hiding this comment.
We should also consider to reference the gRPC spec file or json representation.
| it('provides gRPC interface for decorated controllers', async () => { | ||
| @service(helloService.Greeter.service) | ||
| class MyController implements Greeter { | ||
| hello({name}: HelloRequest): HelloResponse { |
There was a problem hiding this comment.
A method-level gRPC decorator is useful too.
There was a problem hiding this comment.
Method level dependency injection will be interesting too, for example:
hello(@inject('grpc.request#name') name: string,
@inject('grpc.request#metadata.clientid') clientId: string): HelloResponse {
...
}
|
|
||
| // TODO(bajtos) serviceSpec needs to be typed | ||
| export function service(serviceSpec: any) { | ||
| return function(controllerCtor: Function) { |
There was a problem hiding this comment.
Is it important that we append the property to the target as-is, or can we extend it with the decorator?
export function service(serviceSpec: any) {
return function newClass<T extends {new(...args: any[]):{}}>(target: T) {
return class extends target {
grpcService: any = serviceSpec;
}
}
};
There was a problem hiding this comment.
Good point. I think it should be ok to return a new class in the decorator. But then what are the downsides of appending the static property the existing controller class? Is the extra complexity worth it?
One of my concerns is that controllers decorated with @services will have anonymous class name or the same class name newClass for all controllers. This will make (error) stack traces rather unhelpful, as we are already experiencing with juggler models that all have the sam class name ModelConstructor (see lib/model-builder.js).
|
The last commit is adding very basic support for gRPC Sequence. I'll post a longer comment to #521 The tests are crashing with Segmentation Fault, I was not able to track down the exact source of the problem yet. Another reason why to be careful about the |
|
Closing without merging, the purpose of this PR was to verify that gRPC can be implemented. |
Add an initial version of gRPC support, using API/protocol-first approach.
Connect to #521