Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: fix grpc and proto-loader deps #3337

Merged
merged 6 commits into from
Oct 21, 2022

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Oct 14, 2022

  • Remove unused @grpc/proto-loader dependencies or move them to dev deps where possible.
  • Update grpc-js and proto-loader versions: most of the packages contain grpc-js with ^1.5.9 versioning, which might install (currently) up to grpc-js 1.7, which depends on proto-loader 0.7.x. However proto-loader dependencies were pinned to patch versions, this would cause duplicate proto-loader package installs.
  • Remove exact grpc-js and proto-loader versions from opentelemetry-instrumentation-grpc.

@seemk seemk requested a review from a team October 14, 2022 21:43
@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Merging #3337 (93b51e3) into main (292a53d) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3337   +/-   ##
=======================================
  Coverage   93.47%   93.47%           
=======================================
  Files         244      244           
  Lines        7325     7325           
  Branches     1515     1515           
=======================================
  Hits         6847     6847           
  Misses        478      478           

@@ -48,6 +48,7 @@
},
"devDependencies": {
"@babel/core": "7.16.0",
"@grpc/proto-loader": "^0.7.3",
Copy link
Member

Choose a reason for hiding this comment

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

Is @grpc/proto-loader needed in the dev dependencies even @grpc/grpc-js is a dependency?
I thought @grpc/grpc-js ensures that the correct/matching version of @grpc/proto-loader is installed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since test code requires it explicitly, can we be sure @grpc/grpc-js will depend on it in the future? It might introduce potential breakage 🤔

Copy link
Member

Choose a reason for hiding this comment

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

if we depend on a package directly then it should be in the package.json. Even if it is transitively required by another package npm handles the dedupe when required.

Copy link
Member

Choose a reason for hiding this comment

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

If it is within our dependency dedupe might fail if ranges don't match. Usually we pin our dev deps (otherwise renovate bot complains) which doesn't match to what grpc-js does.

Anyhow I agree it's better to add what we need and update in sync with grpc-js. Maybe renovate bot can support here but I guess it's not so easy to configure it for our needs.

@legendecas legendecas merged commit 30a81bd into open-telemetry:main Oct 21, 2022
@seemk seemk deleted the grpc-update-1 branch November 9, 2022 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants