agreatfool/grpc_tools_node_protoc_ts

Argument of type 'Handler' is not assignable to parameter of type 'UntypedServiceImplementation'

yellowandy opened this issue ยท 16 comments

Upgraded to the latest and started getting the error message above when using the addService call. Noticed that in your example you are ignoring the compilation issue with the ts-ignore directive, is this a known issue?

server.addService(BookServiceService, new ServerImpl());

Do you mean this one? #60

Seems not, could you share some sample codes or proto file? Also the versions you are using. Including the grpc_tools_node_protoc.

Yes, it's a known issue. Let me have a look what I can do to solve | improve it.

New version v5.1.0 has been published. Closing.

@agreatfool can you link the code changes to this issue?

Thanks. Great to see this fixed. It looks like we now have Type parity across the popular tools:

  • grpc-proto-loader
  • ts-protoc-gen
  • grpc_tools_node_protoc_ts

I'll update my example in https://github.com/badsyntax/grpc-js-types/tree/master/examples/grpc_tools_node_protoc_ts#issues-with-this-approach to show this latest change.

I'm not sure this is fixed. After updating to 5.1.0, I started getting TS compilation errors due to this change. VS Code expects the following to be in my implementation:

class MyServerImpl implements IMyServiceServer {
    [name: string]: grpc.UntypedHandleCall; // <== This line is expected...

    // But if I have properties / methods that are not RPC-handling like...
    data: Foo = ...;
    someOtherFunction() {...}
}

I get the following compiler error: Property 'data' of type 'Foo' is not assignable to string index type 'HandleCall<any, any>'.ts(2411)

@codedread I did't expect the case of private attributes of the implementation class. Seems this breaking change affected a lot. I will have a check with it, maybe revert this.

Fixing one issue and introduce another is not good. Sorry for the inconvenience.

New version v5.1.1 has been published, only some docs added.

I recommend the Object style implementation of server side:

const Impl: IBookServiceServer = {
    getBook: (call: grpc.ServerUnaryCall<GetBookRequest, Book>, callback: sendUnaryData<Book>): void => {},

    getBooks: (call: grpc.ServerDuplexStream<GetBookRequest, Book>): void => {},

    getBooksViaAuthor: (call: grpc.ServerWritableStream<GetBookViaAuthor, Book>): void => {},

    getGreatestBook: (call: grpc.ServerReadableStream<GetBookRequest, Book>, callback: sendUnaryData<Book>): void => {},
};

const server = new grpc.Server();
server.addService(BookServiceService, Impl);

Read more here.

I think this is the right approach. While no-one likes UntypedServiceImplementation, it unfortunately cannot be changed now to keep backwards compatibility. And best to stick with it to keep type parity. I did some research to see if we can adjust the types in a generic/non-breaking way to support class implementations, but wasn't able to find a solution. There's been some discussion about this here: grpc/grpc-node#1474 (comment)

@badsyntax That's pretty cool. Thanks for the sharing.

Sadly, not supporting the class syntax means at least two not-great things:

  • there is no "instance" of the service so testing becomes more complicated as you have to carefully clear out state of the service.
  • there is no constructor step which I can use as a convenient spot for dependency injection or to mock out dependencies in unit tests

Yes, I can work around this but it is not great. My current TypeScript:

import { Logger} from 'logger';
class MyService extends IMyService {
  logger: Logger;
  constructor() {
    this.logger = new Logger();
  }
  // rest of GRPC things here...
}

In my test, I want to mock out Logger as a dependency:

import { * as loggerModule } from 'logger';
describe('', () => {
  const myService: MyService;
  beforeEach(() => {
    importMock.mockClassInPlace(commonModule, 'Logger');
    myService = new MyService(); // Each test gets a new instance of a service...
  });
  // All my tests here that don't have to worry about the Logger dependency...
}

Currently it seems this is the only possible way to do even from official comments : grpc/grpc-node#1474 (comment).

For class style, maybe we can make something like this (idea, not tested):

class ServiceWrapper {
  // class attributes & other stuff
  // ...

  public svcImpl: IBookServiceServer = {
    getBook: (call: grpc.ServerUnaryCall<GetBookRequest, Book>, callback: sendUnaryData<Book>): void => {},
    getBooks: (call: grpc.ServerDuplexStream<GetBookRequest, Book>): void => {},
    getBooksViaAuthor: (call: grpc.ServerWritableStream<GetBookViaAuthor, Book>): void => {},
    getGreatestBook: (call: grpc.ServerReadableStream<GetBookRequest, Book>, callback: sendUnaryData<Book>): void => {},
  };
}

const impl = new ServiceWrapper().svcImpl;
const server = new grpc.Server();
server.addService(BookServiceService, impl);

Hi everyone, I hit the same issue with the typings for addService. I'm thinking that a backwards compatible fix would be to generate an additional interface type without UntypedServiceImplementation, e.g.

export interface ITypedGreeterServer {
  sayHello: grpc.handleUnaryCall<protos_greeter_pb.HelloRequest, protos_greeter_pb.HelloReply>;
}

export interface IGreeterServer extends ITypedGreeterServer, grpc.UntypedServiceImplementation {
}

This would allow ITypedGreeterServer to be implemented in a class.

Backwards-compatible type-safe addService could be achieved with an override:

addService(service: ServiceDefinition, implementation: UntypedServiceImplementation): void;
addService<TypedServiceImplementation extends Record<any,any>>(service: ServiceDefinition, implementation: TypedServiceImplementation): void;

Then you can use it like this:

class GreeterServiceImpl implements services.ITypedGreeterServer {
   ...
}

const impl = new GreeterServiceImpl();
server.addService<services.ITypedGreeterServer>(services.GreeterService, impl);

and everything else should just keep working.


Alternatively, if you want to get ITypedGreeterServer today, you can use this helper in your own code:

type KnownKeys<T> = {
  [K in keyof T]: string extends K ? never : number extends K ? never : K
} extends { [_ in keyof T]: infer U } ? U : never;

type KnownOnly<T extends Record<any,any>> = Pick<T, KnownKeys<T>>;

Which gets you:

type ITypedGreeterServer = KnownOnly<services.IGreeterServer>;

@jahewson is any chance you have a more complete example you can share with the generic magic you're suggesting?

Here's an attempt of mine but it's failing:

type KnownKeys<T> = {
  [K in keyof T]: string extends K ? never : number extends K ? never : K
} extends { [_ in keyof T]: infer U } ? U : never;

type KnownOnly<T extends Record<any,any>> = Pick<T, KnownKeys<T>>;

type ITypedArticleServer = KnownOnly<IArticleServerServer>;

class ArticleServer implements ITypedArticleServer  {
    key: string
    secret: string

    constructor(key: string, secret: string) {
        this.key = key
        this.secret = secret
    }

    async getArticle(_: ServerUnaryCall<Empty, Empty>, callback: sendUnaryData<Empty>): Promise<void> {
        const files = await fleekStorage.listFiles({
            apiKey: this.key,
            apiSecret: this.secret,
            getOptions: [
                'bucket',
                'key',
                'hash',
                'publicUrl'
            ],
        })
        console.log(files)
        callback(null, new Empty())
    }
}

const server = async (yargs: yargs.Arguments) => {
    const apiKey = String(yargs['key']);
    const apiSecret = String(yargs['secret']);
    const port = yargs['port']

    const server = new grpc.Server();
    server.addService(ArticleServerService, new ArticleServer(apiKey, apiSecret)); // <--------------- this errors out!!!

    server.bindAsync(`localhost:${port}`, grpc.ServerCredentials.createInsecure(), (err, port) => {
        if (err) {
            throw err;
        }
        console.log(`Listening on ${port}`);
        server.start();
    });

}

And here's the error I get on the .addService line: TS2345: Argument of type 'ArticleServer' is not assignable to parameter of type 'UntypedServiceImplementation'.   Index signature is missing in type 'ArticleServer'.

EDIT: ah, looks like I need to add a typed override to the grpc.Server class. Is that right? If so, how? What's the implementation of the override?

EDIT 2: ah, looks like I figured it out.

I created a class which extends grpc.Server:

class TypedServerOverride extends grpc.Server {
    addServiceTyped<TypedServiceImplementation extends Record<any,any>>(service: ServiceDefinition, implementation: TypedServiceImplementation): void {
        this.addService(service, implementation)
    }
}

And then when created the server object I do it like so:

    const server = new TypedServerOverride();
    server.addServiceTyped<ITypedArticleServer>(ArticleServerService, new ArticleServer(apiKey, apiSecret));

    server.bindAsync(`localhost:${port}`, grpc.ServerCredentials.createInsecure(), (err, port) => {
        if (err) {
            throw err;
        }
        console.log(`Listening on ${port}`);
        server.start();
    });