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

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

Closed
yellowandy opened this issue Dec 17, 2020 · 16 comments

Comments

@yellowandy
Copy link

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());

@agreatfool

This comment has been minimized.

@agreatfool

This comment has been minimized.

@agreatfool
Copy link
Owner

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

@agreatfool
Copy link
Owner

New version v5.1.0 has been published. Closing.

@badsyntax
Copy link

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

@agreatfool
Copy link
Owner

@badsyntax a2e4dfa

@badsyntax
Copy link

badsyntax commented Dec 29, 2020

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.

@codedread
Copy link

codedread commented Jan 4, 2021

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)

@agreatfool
Copy link
Owner

@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.

@agreatfool agreatfool reopened this Jan 5, 2021
@agreatfool
Copy link
Owner

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.

@badsyntax
Copy link

badsyntax commented Jan 26, 2021

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)

@agreatfool
Copy link
Owner

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

@codedread
Copy link

codedread commented Jan 27, 2021

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...
}

@agreatfool
Copy link
Owner

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);

@jahewson
Copy link

jahewson commented Jan 30, 2021

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>;

@paymog
Copy link
Contributor

paymog commented Jul 30, 2021

@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();
    });

paymog added a commit to paymog/grpc_tools_node_protoc_ts that referenced this issue Aug 2, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Add new docs explaining how to create GRPC services with classes based on some magic generics from agreatfool#79 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants