-
Notifications
You must be signed in to change notification settings - Fork 597
chore(instrumentation-fastify): drop unused devDep @types/express #2962
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(instrumentation-fastify): drop unused devDep @types/express #2962
Conversation
…devDep @types/express
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2962 +/- ##
==========================================
+ Coverage 89.53% 89.79% +0.25%
==========================================
Files 183 188 +5
Lines 8985 9295 +310
Branches 1867 1907 +40
==========================================
+ Hits 8045 8346 +301
- Misses 940 949 +9 🚀 New features to boost your workflow:
|
This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. |
In So the compilation step should fail if the types are not present. But if we list where the dependency is npm ls @types/express
[email protected] /Users/david/Documents/repos/otel/opentelemetry-js-contrib
├─┬ @opentelemetry/[email protected] -> ./packages/instrumentation-express
│ └── @types/[email protected]
├─┬ @opentelemetry/[email protected] -> ./packages/instrumentation-fastify
│ └── @types/[email protected] deduped
└─┬ @opentelemetry/[email protected] -> ./packages/instrumentation-koa
└─┬ @types/[email protected]
└─┬ @types/[email protected]
└── @types/[email protected] deduped We can see that we get the express types as a dependency from IMHO removing |
…dentifying that @types/express *is* used in the test files)
Thanks! Updated. |
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.
Thanks for working on this :)
For some background, in #1804 the
@types/express
dep was removed from instr-express's dependencies after some long discussion. At the time it was moved to a devDep.However,
@types/express
isn't used at all in these packages. Can we drop them?Or is there some slight downside for possible IDE completion when developing express examples or similar in this repo by removing these?
Note that I only bothered looking at these because of this bot PR to update these deps: #2925 (comment)