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(instr-perf-tools): update deps to similar versions as other contrib packages #1967

Closed

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Feb 27, 2024

  • '@opentelemetry/instrumentation' dep should match others in this repo
  • drop the '@types/{mocha,node}' devDeps because I'm not sure they are needed
  • reduce 'mocha' dep to v7 used by all other packages
  • bump /api peer dep to 1.3.0 (the version when api-metrics was merged into api)

  • '@opentelemetry/instrumentation' dep should match others in this repo

I actually not sure if this is cargo-culting.

  • drop the '@types/{mocha,node}' devDeps because I'm not sure they are needed

Is this wrong? Basically all contrib packages include these two packages as devDeps. Are those @types/... packages needed for IDE usage or something? My "I'm not sure they are needed" is based on running npm run compile && npm test.

…trib packages

- '@opentelemetry/instrumentation' dep should match others in this repo
- drop /api devDep: the tests don't currently use it
- drop the '@types/{mocha,node}' devDeps because I'm not sure they are needed
- reduce 'mocha' dep to v7 used by all other packages
- bump /api peer dep to 1.3.0, when api-metrics was merged into api
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Merging #1967 (ddc4150) into main (44b2df0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1967   +/-   ##
=======================================
  Coverage   91.02%   91.02%           
=======================================
  Files         146      146           
  Lines        7491     7491           
  Branches     1501     1501           
=======================================
  Hits         6819     6819           
  Misses        672      672           

@pichlermarc
Copy link
Member

pichlermarc commented Feb 28, 2024

I actually not sure if this is cargo-culting.

  • drop the '@types/{mocha,node}' devDeps because I'm not sure they are needed

Is this wrong? Basically all contrib packages include these two packages as devDeps. Are those @types/... packages needed for IDE usage or something? My "I'm not sure they are needed" is based on running npm run compile && npm test.

@types/node is needed for assert, and @types/mocha is needed for describe, beforeEach, ...

For me, at least, my IDE (JetBrains WebStorm) does not complain, but I think that's because it's being hoisted by other packages in the workspace. Removing the package from the workspace, adding lerna, and npm install-ing it causes it not to compile afterward. 🤔

We usually include all devDependencies explicitly in each package as we try to avoid depending on another package's hoisted dependencies as devDependencies. Another goal was to keep single packages npm install-able, speeding up the workflow when working on only one package. I'm not sure if all arguments still hold (I know working on a single package would require at least lerna to be installed which is not listed as a devDependency in packages, but I think that's the only exception to the rule at the moment).

That being said: the decision to do this was made a long time ago - happy to see that revised it if there's a better alternative. Doing so would probably better suited for another PR, though. 🙂

@pichlermarc
Copy link
Member

bump /api peer dep to 1.3.0 (the version when api-metrics was merged into api)

👍

reduce 'mocha' dep to v7 used by all other packages

👍

'@opentelemetry/instrumentation' dep should match others in this repo

👍

@trentm
Copy link
Contributor Author

trentm commented Mar 5, 2024

Replaced by #1994

@trentm trentm closed this Mar 5, 2024
@trentm trentm deleted the tm-instr-perf-hooks-devDep-versions branch March 5, 2024 20:27
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.

2 participants