Skip to content
This repository was archived by the owner on Nov 18, 2025. It is now read-only.

feat: reexport protobufjs from gax#544

Merged
alexander-fenster merged 2 commits intomasterfrom
reexport-protobufjs
Jul 25, 2019
Merged

feat: reexport protobufjs from gax#544
alexander-fenster merged 2 commits intomasterfrom
reexport-protobufjs

Conversation

@alexander-fenster
Copy link
Contributor

@alexander-fenster alexander-fenster commented Jul 25, 2019

We sometimes need protobufjs in client libraries (e.g. for LRO proto loading). gapic-generator does not do a great job detecting if protobufjs is needed or not for the given client library, which sometimes causes lint errors about unused dependency.

Since protobufjs is an important part of gax, it will always be installed within gax. Let's just reexport it from gax, so that the client library used gax.protobuf.loadSync, gax.protobuf.Root, etc. whenever needed. By doing this, we'll remove protobufjs dependency from the client libraries, and also will make sure there will be no compatibility problems between two protobufjs dependencies.

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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 25, 2019
Copy link
Contributor

@LibanOdowa LibanOdowa left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

LGTM. I ❤️ the idea of being able to remove almost all explicit dependencies in the gapic generated libraries. We're actually really close.

@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5429ad9). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #544   +/-   ##
=========================================
  Coverage          ?   89.86%           
=========================================
  Files             ?       52           
  Lines             ?     3512           
  Branches          ?      267           
=========================================
  Hits              ?     3156           
  Misses            ?      290           
  Partials          ?       66
Impacted Files Coverage Δ
test/grpc.ts 93.37% <100%> (ø)
src/index.ts 90.9% <100%> (ø)

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 5429ad9...1fc5e8b. Read the comment docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants