-
Notifications
You must be signed in to change notification settings - Fork 807
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
Conversation
Codecov Report
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@grpc/proto-loader
dependencies or move them to dev deps where possible.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.opentelemetry-instrumentation-grpc
.