Typings issue in version 1.13.0

[gunblues]

If I run tsc -d on my Typescript source I get the following message:

node_modules/grpc/index.d.ts:69:50 - error TS2314: Generic type 'Message<T>' requires 1 type argument(s).

69     [name: string]: GrpcObject | typeof Client | Message;
                                                    ~~~~~~~

grpc-node version: 1.13.0
Typescript version: 2.9.2
tsconfig.json compiler options:

"compilerOptions": {
    "target": "es6",
    "module": "commonjs",
    "moduleResolution": "node",
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "allowSyntheticDefaultImports": true
  }

I have found this issue had been fixed before - #176
it had been restored back after this commit - abb05f0#diff-84a425dc635cd3214797bf694fad81e8

[nicolasnoble]

There seems to be a tug-of-war on this one, and a more thorough discussion is needed. See the following pull requests:

#393
#422

[murgatroid99]

@gunblues Would you mind sharing the output of npm ls in your project directory?

[gunblues]

@murgatroid99 I have reproduced this issue via https://github.com/gunblues/grpc-node-typing-issue

[FedeBev]

This package, at the version 1.13, use protobufjs 5; so you should import @types/protobufjs@^5.x.x in you project and not the latest one.

[nicolasnoble]

What's the story exactly when one uses @grpc/proto-loader which uses protobufjs 6 ?

[andrew8er]

As far as I see it, grpc-node is both compatible with protobufjs 5 and 6 (I use it with version 6). So neither version of Message is right. This whole adding and removing of the generic parameter does not fix the problem:

grpc-node should not make any assumptions on the encoding format and thus should not depend directly on protobufjs, regardless of version.

[nicolasnoble]

Yes, you're correct - what's your suggestion to do this properly ?

[andrew8er]

On a high level, there should be an adapter object that is passed to the service description loading mechanism and that takes care of converting from raw buffers to Js objects and vice versa. This adapter would need to live in its own package and depend on its respective wire format library, so users will only drag in the dependencies necessary for their use case.

So we would have packages grpc-codec-protobufjs5, grpc-codec-protobufjs6, maybe even grpc-codec-thrift, grpc-codec-capnproto.

Currently, grpc-node does some guesswork to determine if it is run with protobufjs version 5 or 6 (please correct me, if I'm wrong). This should be done explicitly via an API as mentioned above.

Furthermore, we would need to get rid of any direct dependency on protobufjs in the whole grpc-node package (and consequently in its type definition file). Only the codec adapter would have that dependency.

Actually, I think we are not too far away from that. The Message type occurs only once in the definition file. grpc-node already has serialize<T> and deserialize<T> type.

Btw. has anybody seen a project that uses gRPC with something other than protobuf?

[murgatroid99]

Is this suggestion substantially different from the @grpc/proto-loader library and the addPackageDefinition API that was recently added to the grpc library?

As for removing the protobufjs dependency from grpc, the main problem there is that for various reasons we can't make breaking changes. So we have to work with what we have.

[andrew8er]

Yes, after looking at it, I think it is a step in the right direction, though it handles protobuf messages only, albeit both version 5 and 6 (I think).

Would that mean, that the GrpcObject definition and the load/loadObject functions will be removed eventually? That would fix this issue and also the related issue #176.

That would of course be a breaking change that needs a major version upgrade.

[murgatroid99]

The proto loader library specifically uses Protobuf.js 6. Protobuf.js 5 is outdated, and the grpc library only still uses it for API compatibility reasons. But the loadPackageDefinition function is generic, and can work with any kind of serialization. That API is public, so anyone can write one of the other serialization libraries you suggest.

As I said, we can't make breaking changes, so I wouldn't expect load, loadObject, and GrpcObject to go away in the near future. However, those functions are not implemented in the @grpc/grpc-js library, so people who switch to that will not have to worry about that.

[andrew8er]

The problem is that grpc-node depends on protobufjs-5. It references the Message type from that package in its definition file, so you can only use it with that version without getting a compiler error.

If you are writing a gRPC server, the @grpc/grpc-js package will not help you, since as far as I know, it does not support the server side API. The @grpc/proto-loader package alone will not help you either, since you also will pull in the grpc-node definitions somewhere. That definition file is part of the package's public API for Typescript users, so for them, this was already a breaking change.

A possible solution is to remove the reference to protobufjs in the definition file and instead use a local definition of Message which is compatible with both versions.

Another stop gap solution everyone affected by this issue is to use a customized definition file. You need to configure your Typescript compiler to pick up this version for the grpc module:

{
  "compilerOptions": {
    "baseUrl": ".", // This must be specified if "paths" is.
    "paths": {
      "grpc": ["custom/grpc/definition/file"] // This mapping is relative to "baseUrl"
    }
  }
}

[nicolasnoble]

I think I like the custom Message definition idea, but what exactly would it be like? I can't really think of anything else than something very generic like "object".

[andrew8er]

True, the API of Messagediffers significantly for both versions (comparing protobuf5 and protobuf6). So I would declare an empty facade type and users would be required to cast to their appropriate implementation type.

Typescript has no opaque type, but the new unknown type might be of help.

[kjin]

Apologies for not looking into the Protobuf 5-6 tussle more when I introduced the change back to non-generic Message.

Would it be worthwhile to open a PR to change pb6 types to have a default Message<T extends object = object> type parameter?

[nicolasnoble]

That might be a good quick fix, yes.

[kjin]

protobufjs/protobuf.js#1086

[murgatroid99]

That PR has been merged and published, so this should no longer be a problem if you update

[murgatroid99]

Actually, it hasn't quite made it out yet. Reopening until then.

[nicolasnoble]

And now it has :-D