[Bug] Generated Promise based output inconsistent with grpc-web public API
- Dominant language
- JavaScript
- Stars
- 9.3k
- Forks
- 802
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 5
Description
Hello.
Trying to migrate a project over to official grpc-web, but there are a few things blocking me and my team. I'd like to report my findings.
1) The type definition in `grpc-web/index.d.ts` is different from the one coming from actual npm package. The `unaryCall` method is missing in installed package [NPM_CODE](https://www.npmjs.com/package/grpc-web?activeTab=code), but present in this repository [grpc-web/index.d.ts](https://github.com/grpc/grpc-web/blob/master/packages/grpc-web/index.d.ts).
2) The `unaryCall` definition in [grpc-web/index.d.ts](https://github.com/grpc/grpc-web/blob/master/packages/grpc-web/index.d.ts), does not follow [THE IMPLEMENTATION](https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/grpcwebclientbase.js#L188). The last `options` argument is not present in the definition.
3) The generated output (both typescript and commonjs+dts) does not include this `options` argument in `client_.unaryCall` call, as visible [HERE](https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/generator/grpc_generator.cc#L711-L719). This is quite important since the `options` argument carries abort signal.
4) There is a difference in generated output API and the docs. JS version has separated `*ServiceClient` and `*ServicePromiseClient`, TS output generates single `*ServiceClient` class with method overloads.
Proposition.
The above issues might get just fixed, which from first look at the source code doesn't look too complicated. Only the part with overloading might cause some problems in typescript generated function body, because of extra logic needed to figure out if the argument is `callback` function, or `options` object. However, there might be a way to overcome this complications, if there is a chance to change the generated API.
Could the generated API follow a little different approach, exposing separate methods for different `call` methods? Instead of separate client classes and/or method overloading, have a single class with `methodNameUnary` and `methodNamePromise` and `methodNameRpc`? One calling `unaryCall` other calling `thenableCall`, etc.
I believe this way would be more explicit and also might be a bit easier to maintain in the generator. I also think, this proposed change could be done in a non-breaking way, leaving current API as it is and just adding new explicit ones next to that.
👋
Contributor guide
Research direction
Compare packages/grpc-web/index.d.ts with the published npm definition, then inspect javascript/net/grpc/web/grpcwebclientbase.js and the generator logic at javascript/net/grpc/web/generator/grpc_generator.cc:711-719. Trace the TypeScript and CommonJS+d.ts generated outputs and their documented client APIs. Done means the definitions and generated calls consistently expose the options argument and the intended client API.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, typescript
- Domain
- api
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100