smolijar/protocat

Version mismatch causes compilation and runtime errors with the Client

rrichardson opened this issue · 3 comments

rrichardson commented

Just a heads-up, when following the directions in the docs, I am able to create a Server, but the Client fails to build due to

src/index.ts:6:31 - error TS2345: Argument of type 'typeof CatRegisterClient' is not assignable to parameter of type 'Record<string, new (...args: any[]) => Client> | (new (...args: any[]) => Client)'.
  Types of construct signatures are incompatible.
    Type 'new (address: string, credentials: ChannelCredentials, options?: Partial<ClientOptions> | undefined) => CatRegisterClient' is not assignable to type 'new (...args: any[]) => Client'.
      Type 'CatRegisterClient' is missing the following properties from type 'Client': [CHANNEL_SYMBOL], [INTERCEPTOR_SYMBOL], [INTERCEPTOR_PROVIDER_SYMBOL]

6   const client = createClient(CatRegisterClient, "localhost")

It seems that the version mismatch between the latest @grpc/grpc-js (1.2.5)
and the current protocat: (1.2.2) was enough to cause this problem.

I haven't dug into the details, but it seems a bit odd that a patch version would break API compatibility like that.

rrichardson commented

Also, after fixing that bit, I run into this:

(node:178225) UnhandledPromiseRejectionWarning: TypeError: Channel credentials must be a ChannelCredentials object
    at new ChannelImplementation (/var/home/rick/Projects/letter/testgrpc/client/node_modules/@grpc/grpc-js/build/src/channel.js:69:19)
    at new Client (/var/home/rick/Projects/letter/testgrpc/client/node_modules/@grpc/grpc-js/build/src/client.js:58:36)
    at new ServiceClientImpl (/var/home/rick/Projects/letter/testgrpc/client/node_modules/@grpc/grpc-js/build/src/make-client.js:58:5)
    at Object.createClient (/var/home/rick/Projects/letter/protocat/dist/lib/client/client.js:82:37)
    at /var/home/rick/Projects/letter/testgrpc/client/lib/index.js:47:41
    at step (/var/home/rick/Projects/letter/testgrpc/client/lib/index.js:33:23)
    at Object.next (/var/home/rick/Projects/letter/testgrpc/client/lib/index.js:14:53)
    at /var/home/rick/Projects/letter/testgrpc/client/lib/index.js:8:71
    at new Promise (<anonymous>)
    at __awaiter (/var/home/rick/Projects/letter/testgrpc/client/lib/index.js:4:12)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:178225) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:178225) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

This was due to a version mismatch in the ChannelCredentials object, I guess. I fixed it by initializing the ChannelCredentials.createInsecure() and passing it in the optional param.

smolijar commented

Thanks for report. I do realize the client is little bit fragile. It started as a PoC, but became somewhat practical so i decided to release it along with the server.

ProtoCat uses at few places non-public API, which enables it to do default response message instantiation and advanced caching, that's why there is a specific version lock in package.json. Both of these problems originate from version conflict.

The TS problem is inherent from the client dynamic types. The other issue could be "patched" by simply requesting the argument, since many users will probably have a custom wrapper with services etc.

The way I see it there are several options how to solve the issue:

  1. Remove the client altogether
  2. Be more explicit about the version lock in docs, or mark the client only as experimental in docs
  3. Refactor the type inference to avoid extending direct type (which fails on version mismatch), but making them more complex and harder to maintain (actually solves the compilation problem)
  4. Require the argument (there is not even a good way to do the validation, because protocat cannot access the grpc that will be used and therefore cannot check the correct thing is passed down)

I acknowledge it is an issue, but there is no obvious good approach IMO. If the issue get's some more attention and it indeed turns out to be a blocker for many users I will make it my priority.

rrichardson commented

Understood.
I will probably just skip the protocat client and wrap the generated '*Client'

Thanks for making a great library.

  • ❤️1