-
Notifications
You must be signed in to change notification settings - Fork 821
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
fix(opentelemetry-sdk-node): move nock to dev dependencies #2219
Conversation
Signed-off-by: nflaig <[email protected]>
|
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.
Out of curiosity did you just happen to notice this or is there a tool that can produce a warning about this?
Codecov Report
@@ Coverage Diff @@
## main #2219 +/- ##
==========================================
- Coverage 92.71% 92.69% -0.02%
==========================================
Files 142 142
Lines 5120 5120
Branches 1049 1049
==========================================
- Hits 4747 4746 -1
- Misses 373 374 +1
|
@johnbley I happend to notice it while reviewing the code but npm-check could potentially be used to do this in an automated way, for example cp packages/opentelemetry-sdk-node/package.json packages/opentelemetry-sdk-node/src/package.json
npm-check packages/opentelemetry-sdk-node/src --production
rm packages/opentelemetry-sdk-node/src/package.json would produce the following nock 😎 MAJOR UP Major update available. https://github.com/nock/nock#readme
npm install --save [email protected] to go from 12.0.3 to 13.0.11
😕 NOTUSED? Still using nock?
Depcheck did not find code similar to require('nock') or import from 'nock'.
Check your code before removing as depcheck isn't able to foresee all ways dependencies can be used.
Use --skip-unused to skip this check.
To remove this package: npm uninstall --save nock This does not seem to be the most robust solution I guess but it would have worked in this specific case. |
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.
lgtm, nice finding
This PR moves
nock
fromdependencies
todevDependencies
since the library is only required during development.