Skip to content
This repository was archived by the owner on Dec 8, 2021. It is now read-only.

feat: allow passing grpc in service constructor#241

Merged
JustinBeckwith merged 4 commits intomasterfrom
pass-grpc
May 10, 2019
Merged

feat: allow passing grpc in service constructor#241
JustinBeckwith merged 4 commits intomasterfrom
pass-grpc

Conversation

@alexander-fenster
Copy link
Contributor

This PR prepares us to the Global Switch to @grpc/grpc-js. Making it possible to pass another grpc implementation for those packages that are not yet ready to switch.

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #241 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #241      +/-   ##
=========================================
+ Coverage   92.18%   92.3%   +0.12%     
=========================================
  Files           4       4              
  Lines         307     312       +5     
  Branches       60      62       +2     
=========================================
+ Hits          283     288       +5     
  Misses         15      15              
  Partials        9       9
Impacted Files Coverage Δ
src/service.ts 97.03% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2e03d3...0c63d7b. Read the comment docs.


if (config.grpc) {
this.grpc = config.grpc;
this.grpcVersion = config.grpcVersion || 'grpc/unknown';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we had a way to track specifically which grpc module is getting used, and what version. I think we're going to want that data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot - I asked about it a few days ago and there is no way to tell from inside the code if we are using this or that version of gRPC. Asking clients to pass a version is probably the best solution here. Given that we control all clients, we can make sure we do pass the version all the time.

Alternatively, I can throw if grpc is passed and grpcVersion is not. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only alternative I can think of here is having a common interface for grpc (core and js) that effectively returns the version and distribution.

Would you mind filing a feature request against gRPC? Should be additive. I'd be ok with what's here for now, but I just want a plan to have better data as we go.

@JustinBeckwith JustinBeckwith merged commit 1815d40 into master May 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants