Skip to content

Change from class methods to arrow functions#92

Merged
thesayyn merged 4 commits intothesayyn:masterfrom
koblas:master
Oct 4, 2021
Merged

Change from class methods to arrow functions#92
thesayyn merged 4 commits intothesayyn:masterfrom
koblas:master

Conversation

@koblas
Copy link
Copy Markdown
Contributor

@koblas koblas commented Oct 2, 2021

One option for making the calls into promises is to use promisify however, the current codebase generate class methods rather than arrow functions with means that you need to do a bind prior to calling promisify.

import { promisify } from 'util';

// code omitted

/** this is what the change enables */
const addTodo = promisify(client.addTodo)

/** code before the change */
const addTodo = promisify(client.addTodo.bind(client))

From output of test/rpcs.ts

interface GrpcUnaryServiceInterface<P, R> {
    (message: P, metadata: grpc_1.Metadata, options: grpc_1.CallOptions, callback: grpc_1.requestCallback<R>): grpc_1.ClientUnaryCall;
    (message: P, metadata: grpc_1.Metadata, callback: grpc_1.requestCallback<R>): grpc_1.ClientUnaryCall;
    (message: P, options: grpc_1.CallOptions, callback: grpc_1.requestCallback<R>): grpc_1.ClientUnaryCall;
    (message: P, callback: grpc_1.requestCallback<R>): grpc_1.ClientUnaryCall;
}
interface GrpcStreamServiceInterface<P, R> {
    (message: P, metadata: grpc_1.Metadata, options?: grpc_1.CallOptions): grpc_1.ClientReadableStream<R>;
    (message: P, options?: grpc_1.CallOptions): grpc_1.ClientReadableStream<R>;
}
interface GrpWritableServiceInterface<P, R> {
    (metadata: grpc_1.Metadata, options: grpc_1.CallOptions, callback: grpc_1.requestCallback<R>): grpc_1.ClientWritableStream<P>;
    (metadata: grpc_1.Metadata, callback: grpc_1.requestCallback<R>): grpc_1.ClientWritableStream<P>;
    (options: grpc_1.CallOptions, callback: grpc_1.requestCallback<R>): grpc_1.ClientWritableStream<P>;
    (callback: grpc_1.requestCallback<R>): grpc_1.ClientWritableStream<P>;
}
interface GrpcChunkServiceInterface<P, R> {
    (metadata: grpc_1.Metadata, options?: grpc_1.CallOptions): grpc_1.ClientDuplexStream<P, R>;
    (options?: grpc_1.CallOptions): grpc_1.ClientDuplexStream<P, R>;
}
export abstract class UnimplementedStorageService {
   /** omitted - no changes **/
}
export class StorageClient extends grpc_1.makeGenericClientConstructor(UnimplementedStorageService.definition, "Storage", {}) {
    constructor(address: string, credentials: grpc_1.ChannelCredentials, options?: Partial<grpc_1.ChannelOptions>) {
        super(address, credentials, options)
    }
    query: GrpcStreamServiceInterface<Query, Query> = (message: Query, metadata?: grpc_1.Metadata | grpc_1.CallOptions, options?: grpc_1.CallOptions): grpc_1.ClientReadableStream<Query> => {
        return super.query(message, metadata, options);
    };
    get: GrpcUnaryServiceInterface<Query, _Object> = (message: Query, metadata: grpc_1.Metadata | grpc_1.CallOptions | grpc_1.requestCallback<_Object>, options?: grpc_1.CallOptions | grpc_1.requestCallback<_Object>, callback?: grpc_1.requestCallback<_Object>): grpc_1.ClientUnaryCall => {
        return super.get(message, metadata, options, callback);
    };
    put: GrpWritableServiceInterface<Put, _Object> = (metadata: grpc_1.Metadata | grpc_1.CallOptions | grpc_1.requestCallback<_Object>, options?: grpc_1.CallOptions | grpc_1.requestCallback<_Object>, callback?: grpc_1.requestCallback<_Object>): grpc_1.ClientWritableStream<Put> => {
        return super.put(metadata, options, callback);
    };
    chunk: GrpcChunkServiceInterface<Chunk.Query, Chunk> = (metadata?: grpc_1.Metadata | grpc_1.CallOptions, options?: grpc_1.CallOptions): grpc_1.ClientDuplexStream<Chunk.Query, Chunk> => {
        return super.chunk(metadata, options);
    };
}

@thesayyn
Copy link
Copy Markdown
Owner

thesayyn commented Oct 3, 2021

Hey, there this looks promising. Have you seen the EXPERIMENTAL_FEATURES flag that generates unary rpcs as Promises?

@thesayyn
Copy link
Copy Markdown
Owner

thesayyn commented Oct 3, 2021

@koblas
Copy link
Copy Markdown
Contributor Author

koblas commented Oct 3, 2021

I have seen that, I realize that I didn't migrate that to arrow functions. Will update shortly.

My second PR that I was going to send was moving the environment variable version of creating promises to proper protoc based flags. Though I could drop that in here as well.

@koblas
Copy link
Copy Markdown
Contributor Author

koblas commented Oct 3, 2021

I'll update the PR later today with the Promise method migrated over as well. Also throwing in the ability to do:

protoc
    --ts_opt=promise

So it doesn't feel experimental.

@thesayyn
Copy link
Copy Markdown
Owner

thesayyn commented Oct 3, 2021

I would prefer something that is less ambiguous perhaps --ts_out=unary_rpc_promise

@thesayyn
Copy link
Copy Markdown
Owner

thesayyn commented Oct 3, 2021

Also, the reason why i inlined these types was that we were using methods. I believe that these types are already present in @grpc/grpc-js. hence we should use those instead of generating them from scratch

@koblas
Copy link
Copy Markdown
Contributor Author

koblas commented Oct 4, 2021

Dug around in all of the @grpc/grpc-js import files and while they have some gernerics for the server side, they have not defined a generic for any of the calling patterns. Maybe I missed something, but noticed at a few places they just took everything and cast it to what they used rather than providing any interfaces to define against. Most of the generics were in the definition of the service rather than the actual calling conventions which are bundled against UntypedHandleCall.

I've added support for two --ts_opt flags also updated the README and the build script to use them.

  • unary_rpc_promise
  • grpc_package

@thesayyn
Copy link
Copy Markdown
Owner

thesayyn commented Oct 4, 2021

This looks so sweet. 🥲 I love the fact that you have introduced prettier. I have always meant to do that.

Seems like some of the tests are failing. probably a node version skew

@thesayyn
Copy link
Copy Markdown
Owner

thesayyn commented Oct 4, 2021

Most of the generics were in the definition of the service rather than the actual calling conventions which are bundled against UntypedHandleCall

Ah yes. @grpc/grpc-js is poorly typed. It would make our lives a lot easier if you could send a PR to that repository introducing these types so we can rely on them instead of generating them. for short term, this is fine.

@koblas
Copy link
Copy Markdown
Contributor Author

koblas commented Oct 4, 2021

Fixed the test failure. You're right node syntax issue...

Curious, is there any reason this is in pure JavaScript rather than TypeScript, which would have smoothed out that issue?

@thesayyn
Copy link
Copy Markdown
Owner

thesayyn commented Oct 4, 2021

In the beginning, the project wasn’t big enough so started with javascript. also didn’t want to deal with typescript compilation.

after it got big, I have added jsdoc types to provide intellisense.

@thesayyn thesayyn merged commit ce7dd99 into thesayyn:master Oct 4, 2021
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

Successfully merging this pull request may close these issues.

2 participants