fix(grpc-js): Add support for impl type to server.addService #1556

slavovojacek · 2020-08-31T19:38:34Z

Adding support for ImplementationType to server.addService(...).

It seems like the original (non-js) grpc type definitions support this - https://github.com/grpc/grpc-node/blob/grpc%401.24.x/packages/grpc-native-core/index.d.ts#L266.

Not sure if there was a reason not to do it in grpc-js?

Thanks!

linux-foundation-easycla · 2020-08-31T19:38:37Z

The committers are authorized under a signed CLA.

✅ Slavo Vojacek (38e988e)

murgatroid99

The difference there was just an oversight. I'm fine with making this change.

murgatroid99 · 2020-08-31T20:31:56Z

packages/grpc-js/src/server.ts

+      UntypedHandleCall
+    > = UntypedServiceImplementation
+  >(
+    service: ServiceDefinition<ImplementationType>,


I think this might need to be Partial<ServiceDefinition<ImplementationType>>. A service definition is allowed to be missing some methods.

Don't think Partial<T> is needed here as ServiceDefinition<T> is defined in such a way that allows the service definition to be missing some methods?

export interface UntypedServiceImplementation { [name: string]: UntypedHandleCall; } export type ServiceDefinition< ImplementationType = UntypedServiceImplementation > = { readonly [index in keyof ImplementationType]: MethodDefinition<any, any>; };

But I may be wrong... 🤔

Oh, right, the problem might actually be the other way around: if the implementation object has methods that aren't part of the service definition, those keys will be considered missing in the service definition.

And I think the Partial<T> there solves that problem: the inferred ServiceDefinition<ImplementationType> type may have extra keys corresponding to implementation object methods that are not part of the service, and partial allows those to be omitted from the service definition object.

Will action shortly – just bogged down with other work; sorry!

So I looked into it, pushed new update – that's the best I can manage at this stage. Seems to be working.

slavovojacek · 2020-09-02T15:30:23Z

FYI – faced some issues related to microsoft/TypeScript#15300 with the original implementation.

murgatroid99

I strongly prefer to avoid type assertions wherever possible, because they effectively invalidate the compiler's type checks.

murgatroid99 · 2020-09-02T17:39:38Z

packages/grpc-js/src/server.ts

-    implementation: UntypedServiceImplementation
+  addService<
+    ImplementationType extends {
+      readonly [index in keyof ImplementationType]: UntypedHandleCall;


Does this mean that every member of ImplementationType is an UntypedHandleCall? If so, that is not accurate. Implementation objects are allowed to have other members that are not registered as gRPC method handlers.

I changed this just now.

murgatroid99 · 2020-09-02T17:40:50Z

packages/grpc-js/src/server.ts

@@ -161,7 +165,7 @@ export class Server {
      throw new Error('addService() requires two objects as arguments');
    }

-    const serviceKeys = Object.keys(service);
+    const serviceKeys = Object.keys(service) as Array<keyof ImplementationType>;


This type assertion is not exactly accurate. The service can have keys pointing to method descriptions that are not implemented in the implementation object.

And it's not clear that this provides a significant benefit in terms of narrowing the type, because another type assertion name as string is needed later, and that would be implicit without this type assertion here.

murgatroid99 · 2020-09-02T17:42:03Z

packages/grpc-js/src/server.ts

@@ -189,13 +193,13 @@ export class Server {
      let impl;

      if (implFn === undefined && typeof attrs.originalName === 'string') {
-        implFn = implementation[attrs.originalName];
+        implFn = implementation[attrs.originalName as keyof ImplementationType];


Similar to the previous point, this method may not be implemented in the implementation object, so this is not necessarily an accurate type assertion.

murgatroid99 · 2020-09-02T17:44:02Z

packages/grpc-js/src/server.ts

      }

      if (implFn !== undefined) {
-        impl = implFn.bind(implementation);
+        impl = implFn.bind(implementation as any);


Why is this cast needed at all? It looks to me like bind accepts an unknown, so anything should work.

murgatroid99

Thank you for your contribution.

murgatroid99 · 2020-09-02T22:18:25Z

I'm sorry, but I'm going to have to revert this. It broke my code in a different PR, and that probably means that it has the potential to break other people's code.

slavovojacek · 2020-09-03T07:18:20Z

Thanks for letting me know, I looked at the PR and the error, I do believe #1561 will fix that.

fix(grpc-js): Add support for impl type to server.addService

38e988e

murgatroid99 reviewed Aug 31, 2020

View reviewed changes

chore(Typings): Update types

0d1d5a1

slavovojacek requested a review from murgatroid99 September 2, 2020 17:09

murgatroid99 reviewed Sep 2, 2020

View reviewed changes

fix(grpc): Fix typings

fba1ee0

murgatroid99 added the kokoro:run label Sep 2, 2020

kokoro-team removed the kokoro:run label Sep 2, 2020

murgatroid99 approved these changes Sep 2, 2020

View reviewed changes

murgatroid99 merged commit e21f337 into grpc:master Sep 2, 2020

murgatroid99 mentioned this pull request Sep 2, 2020

Revert "fix(grpc-js): Add support for impl type to server.addService" #1560

Merged

slavovojacek mentioned this pull request Sep 3, 2020

fix(grpc-js): Add support for typed add service #1561

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(grpc-js): Add support for impl type to server.addService #1556

fix(grpc-js): Add support for impl type to server.addService #1556

slavovojacek commented Aug 31, 2020

linux-foundation-easycla bot commented Aug 31, 2020 •

edited

murgatroid99 left a comment

murgatroid99 Aug 31, 2020

slavovojacek Aug 31, 2020

murgatroid99 Aug 31, 2020

murgatroid99 Sep 1, 2020

slavovojacek Sep 2, 2020

slavovojacek Sep 2, 2020

slavovojacek commented Sep 2, 2020

murgatroid99 left a comment

murgatroid99 Sep 2, 2020

slavovojacek Sep 2, 2020

murgatroid99 Sep 2, 2020

slavovojacek Sep 2, 2020

murgatroid99 Sep 2, 2020

slavovojacek Sep 2, 2020

murgatroid99 Sep 2, 2020

slavovojacek Sep 2, 2020

murgatroid99 left a comment

murgatroid99 commented Sep 2, 2020

slavovojacek commented Sep 3, 2020 •

edited

fix(grpc-js): Add support for impl type to server.addService #1556

fix(grpc-js): Add support for impl type to server.addService #1556

Conversation

slavovojacek commented Aug 31, 2020

linux-foundation-easycla bot commented Aug 31, 2020 • edited

murgatroid99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavovojacek commented Sep 2, 2020

murgatroid99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murgatroid99 left a comment

Choose a reason for hiding this comment

murgatroid99 commented Sep 2, 2020

slavovojacek commented Sep 3, 2020 • edited

linux-foundation-easycla bot commented Aug 31, 2020 •

edited

slavovojacek commented Sep 3, 2020 •

edited