Add TypeScript typings to gRPC native core package #52

Crevil · 2017-09-29T09:43:01Z

Following @murgatroid99's request I've moved the PR grpc/grpc#12755 to this repo instead.

From the original PR:

This is a follow up of grpc/grpc#11020. I've accidentally deleted the old gRPC fork I had as I had totally forgotten this PR.

This is a remake with all the latest API changes and the comments from the former PR added.
Further more, I've created a test repo to verify that the type comply with the documented API. It is located here for now: https://github.com/lunarway/grpc-ts-test
If it can fit into the structure of the repo, we could incorporate it to make sure the type and tests stay up to date? Let me know if there is anything I can do in that case.

From the initial PR:

This PR closes grpc/grpc#8233
It adds the most basic typings for TypeScript of the public API of grpc for Node.js.

All details are not defined in this, as I'm no expert in reading the native parts of the module. I've used these definitions for a while and trusts the API. Let me know if anything needs to be corrected or otherwise modified.

All comments etc. are from the existing documentations.

Example usage:

import * as grpc from 'grpc';

const server = new grpc.Server();
server.bind('0.0.0.0:7200', grpc.ServerCredentials.createInsecure());
server.start();

murgatroid99

For the APIs that intersect, this needs to be consistent with the type definitions in packages/grpc-js-core. Can you look there and see if anything there is inconsistent with anything you wrote here?

murgatroid99 · 2017-09-29T16:29:45Z

packages/grpc-native-core/index.d.ts

+   * Callback function passed to server handlers that handle methods with
+   * unary responses.
+   */
+  type sendUnaryData = (error: ServiceError | null, value: any, trailer?: Metadata, flags?: number) => void;


This ��null string appears a few times in this file, and it looks like an error.

Yeah, that sure looks weird... Let me get to that. And I will give the API a look in grpc-js-core as well to make sure we are in line.

murgatroid99 · 2017-09-29T20:52:38Z

packages/grpc-native-core/index.d.ts

+   */
+  export interface GrpcObject {
+    [namespace: string]: {
+      [service: string]: typeof Client,


There isn't exactly one namespace level; there can actually be 0 or more namespaces, and there are also message classes in there, so it should really be something like

import { Message } from "protobufjs"; export interface GrpcObject { [name: string]: GrpcObject | typeof Client | Message, }

I'd prefer not to mention the message classes, but without them the type definition wouldn't accurately represent the actual API.

Got it. I was a bit confused about this part when looking at the docs. I'll clear it up.

murgatroid99 · 2017-09-29T20:56:10Z

packages/grpc-native-core/index.d.ts

+   * of the grpc.logVerbosity map.
+   * @param verbosity The minimum severity to log
+   */
+  export function setLogVerbosity(verbosity: number): void;


The logVerbosity enum is exported below. Should the argument to this function use that enum?

You'r right. This could very well be the enum.

murgatroid99 · 2017-09-29T20:59:46Z

packages/grpc-native-core/index.d.ts

+     * @param implementation Map of method names to method implementation
+     * for the provided service.
+     */
+    addProtoService(service: Service, implementation: { [name: string]: handleCall }): void;


I know it's not currently documented this way, but the first argument to this function can actually be a Service (from Protobuf.js), or a ServiceDefinition object (defined below), or a Service object from Protobuf.js 6 (though it will be difficult to get that type definition here). This is important because the Client#service instance variable is a valid argument to this function.

murgatroid99 · 2017-09-29T21:01:54Z

packages/grpc-native-core/index.d.ts

+     * @param creds Server credential object to be used for SSL. Pass an
+     * insecure credentials object for an insecure port.
+     */
+    bind(port: string, creds: ServerCredentials): void;


This does not actually return void; it returns a number that is equal to the bound port, or 0 if the operation failed. The lack of documentation of this fact is an error.

murgatroid99 · 2017-09-29T21:05:00Z

packages/grpc-native-core/index.d.ts

+     * @param call The call object associated with the request
+     * @param metadata The request metadata from the client
+     */
+    constructor(call: Call, metadata: Metadata);


If this isn't necessary, I would prefer to omit it. This constructor is not explicitly exposed in the API. In addition, the Call type is defined elsewhere as the union of the client call object types, but that is not the correct type for this argument. The argument to this constructor is another type that is also not exposed in the API.

I'll remove them.

murgatroid99 · 2017-09-29T21:05:33Z

packages/grpc-native-core/index.d.ts

+     * @param serialize Serialization function for requests
+     * @param deserialize Deserialization function for requests
+     */
+    constructor(call: any, metadata: Metadata, serialize: serialize, deserialize: deserialize);


As mentioned above, this constructor should not be in the type definition

murgatroid99 · 2017-09-29T21:06:31Z

packages/grpc-native-core/index.d.ts

+    /**
+     * The error code, a key of `grpc.status`
+     */
+    code: number;


The type of code should be the status enum defined below.

murgatroid99 · 2017-09-29T21:10:13Z

packages/grpc-native-core/index.d.ts

+     * @param googleCredential The Google credential object to use
+     * @return The resulting credentials object
+     */
+    createFromGoogleCredential(googleCredential: GoogleCredential): CallCredentials;


I don't believe that GoogleCredential is defined in this file, which is not surprising because gRPC does not explicitly depend on the library that that type comes from. That library is google-auth-library. I think it would be OK to depend just on the type definitions for that library, if such a module exists.

I made a copy of the types into this file, see line 864.
The lib is written in TypeScript so it ships with it's own typings (https://github.com/google/google-auth-library-nodejs/blob/master/package.json#L10)
How would you prefer this? I dep to the lib or a copy of the type?

I see that copy now, and I think that's the right way to handle this. But that's not the right type. I think what I called GoogleCredentials actually corresponds to what's listed as OAuth2Client in that library. The important part of that interface, from the point of view of how this library uses it, is the getRequestMetadata method.

Oh, that's my mistake then. I'll change it to the right one.

I've put som questions into the other comments you left, but I think they've been hidden by the other changes. Would you look into them as well?

murgatroid99 · 2017-09-29T21:12:47Z

packages/grpc-native-core/index.d.ts

+     * An EventEmitter. Used for unary calls.
+     * @param call The call object associated with the request
+     */
+    constructor(call: any);


As with the server-side call classes, I think these constructors should be omitted from the type definition.

murgatroid99 · 2017-09-29T21:23:30Z

packages/grpc-native-core/index.d.ts

+   */
+  type handleClientStreamingCall = (call: ServerReadableStream, callback: sendUnaryData) => void;
+
+  export class ServerReadableStream {


In the actual API, this class inherits from a Readable stream in object mode. Could the fact that those APIs are not in the type definitions cause problems for users? This applies to all of the streaming call types on both the client and server side, as well as the UnaryClientCall type, which inherits from EventEmitter.

I'm not sure about this one. I think we need to explicitly inherit from those types as well otherwise something like this would cause an error.

let c: ServerUnaryCall; c.on("data", () => {});

To make this possible we need the Node.js typings. Should I include them as a dev dependency?

I'm OK with depending on the Node.js typings. Wouldn't we have to include them as a regular dependency, if this index.d.ts we're distributing in the release references it?

Also, it's not as simple as just inheriting from those types. Those types are explicitly written with the assumption that the stream is a byte stream, and it takes some intermediate type redefinitions to make them valid object stream types. You can see what I had to do for this in the other package here: https://github.com/grpc/grpc-node/blob/master/packages/grpc-js-core/src/object-stream.ts

You shouldn't have to do as much as that, because you don't necessarily need strongly typed object streams that restrict inputs and outputs to the specific request and response types. But you at least need equivalents of the intermediate types to allow any in the inputs to write operations and the outputs from read operations.

I'll look into the definitions and see what I can do with it.

All right, I've looked into this. The tests allow for your requested API, so I think we are good for now.

I don't think the changes you made here quite work. As I mentioned, the builtin stream types are written for byte streams. They handle buffers and strings, not arbitrary objects. You need to override the method definitions, like I did in that object stream TypeScript file.

Also, it looks like you didn't change all of the client stream types.

When looking into the Node.js typings it seems to allow the any type for both read results (stream.Readable) and write messages (stream.Writeable).

See this test file where it's possible to write an object: https://github.com/lunarway/grpc-ts-test/blob/master/index.ts#L42

Let me know if I've misunderstood something here?

On the readable side, you can see that the data event restricts the values to Buffer | string: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/index.d.ts#L5067. The Writable type is actually fine.

The first overload of on allows for the any type when reading data events.

readableStream.on("data", (chunk: object) => {});

I've made a test case in this commit: https://github.com/lunarway/grpc-ts-test/commit/23a74d585e2701a993f0b874ad017152cbc41083

connor4312 · 2017-10-02T18:14:17Z

packages/grpc-native-core/index.d.ts

+     * @param credentials Credentials to use to connect to the server
+     * @param {Object} options Options to apply to channel creation
+     */
+    constructor(address: string, credentials: ChannelCredentials, options?: object)


By the way, I creaed typings for the options here

Great, I'll merge them into this file. Thanks!

That's good to know; I would prefer not to include those in this PR. There's some complexity with handling the case where new arguments are added, and I think we'll be able to handle that better in the future after some other changes we're planning to make.

Roger. We leave them out for now then.

… stream pkg

Crevil · 2017-10-06T18:27:07Z

I've identified the following differences from grpc-js-core package.

The method Client#getChannel(): Channel L773 is not exposed in js-core.
Functions not found in js-core
- load src
- loadObject src
- setLogger src
- setLogVerbosity src

murgatroid99 · 2017-10-10T21:49:38Z

Thank you for your contribution. The typings file will be in grpc@1.7, which will be published soon.

ksaldana1 · 2017-10-10T22:22:25Z

Thanks a bunch @Crevil for taking the initiative on this and getting it done! I know I've been eagerly waiting for this, so I'm sure others are too. Thanks also to @murgatroid99 for the code reviews and @connor4312 for providing his typings file to build from.

Crevil · 2017-10-11T05:01:32Z

Thank you for the great review @murgatroid99 .
Let me know if there is anything I can help you with in the future.

chrisalexander55 · 2017-10-24T13:56:47Z

Is this available in the latest build? I am unable to find a type definition file in node_modules/grpc directory. Thanks.

murgatroid99 · 2017-10-24T15:47:37Z

Today (hopefully) we will be publishing a prerelease package for the next version, which includes these changes.

Add TypeScript typings to gRPC native core package

1428315

Crevil mentioned this pull request Sep 29, 2017

Add typings to Node.js grpc module grpc/grpc#11020

Closed

murgatroid99 self-requested a review September 29, 2017 16:24

murgatroid99 reviewed Sep 29, 2017

View reviewed changes

murgatroid99 added the kokoro:run label Sep 29, 2017

kokoro-team removed kokoro:run labels Sep 29, 2017

Remove wrongly encoded spaces

bdbbd53

murgatroid99 mentioned this pull request Sep 29, 2017

Add TypeScript typings to node.js gRPC module (remake) grpc/grpc#12755

Closed

murgatroid99 requested changes Sep 29, 2017

View reviewed changes

Crevil added 2 commits October 1, 2017 10:33

Changes based on review.

4cbf757

Missed a status code enum

cd26599

connor4312 reviewed Oct 2, 2017

View reviewed changes

ksaldana1 mentioned this pull request Oct 3, 2017

Add Optional Metadata Parameter to Service Interface kondi/rxjs-grpc#11

Closed

Crevil added 6 commits October 6, 2017 19:50

Align with js-core package

ae52193

Use the correct Google credentials interface

4647cc1

Add Node typings dependency

56690ca

Make streams extend Readable, Writeable and Duplex types from Node.js…

836f9bd

… stream pkg

Missed a return type on the OAuth2 client interface

11307ac

Order imports correctly

a026502

Added missing streaming implementations

00b4bc6

murgatroid99 approved these changes Oct 10, 2017

View reviewed changes

murgatroid99 added the kokoro:run label Oct 10, 2017

kokoro-team removed the kokoro:run label Oct 10, 2017

murgatroid99 merged commit 0182df2 into grpc:master Oct 10, 2017

ThomWright mentioned this pull request Oct 31, 2017

Include index.d.ts in the published npm package #72

Closed

pocesar mentioned this pull request Jul 20, 2018

[request] gRPC DefinitelyTyped/DefinitelyTyped#11264

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Add TypeScript typings to gRPC native core package #52

Add TypeScript typings to gRPC native core package #52

Conversation

Crevil commented Sep 29, 2017

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

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

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

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

Crevil commented Oct 6, 2017

murgatroid99 commented Oct 10, 2017

ksaldana1 commented Oct 10, 2017 • edited

Crevil commented Oct 11, 2017

chrisalexander55 commented Oct 24, 2017

murgatroid99 commented Oct 24, 2017

ksaldana1 commented Oct 10, 2017 •

edited