-
Notifications
You must be signed in to change notification settings - Fork 615
refactor: hoist all devDeps to root #3032
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
base: main
Are you sure you want to change the base?
refactor: hoist all devDeps to root #3032
Conversation
f104904 to
a80544a
Compare
d186426 to
6e89df1
Compare
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.
Early feedback. I think this is looking pretty good. ;)
- I don't know the karma code well.
- I don't have a feel for the impact of the moduleResolution change in tsconfig.json files.
- (not your fault) Eww on the
assertmodule (because it shadows the core node.js module). Granted that was already installed at the top-level so this PR doesn't impact that at all.
Some questions/notes inline.
| "expect": "29.2.0", | ||
| "nock": "^14.0.0", | ||
| "nyc": "17.1.0", | ||
| "form-data-encoder": "^4.1.0", |
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 this one needed?
The openai dep brings in this package, though a MUCH older one.
[email protected] /Users/trentm/src/opentelemetry-js-contrib
└─┬ @opentelemetry/[email protected] -> ./packages/instrumentation-openai
└─┬ [email protected]
└── [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.
I previously saw issues when running TAV but I just tried again and it's passing. I'll remove.
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.
Okay, noted. I'll take a look. I know TAV well and I'm the component-owner of instr-openai.
| "peerDependencies": { | ||
| "@opentelemetry/api": "^1.3.0" | ||
| "@opentelemetry/api": "^1.3.0", | ||
| "redis": "^5.6.0" |
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.
Are we sure we want a change in peer deps here?
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.
I'm not super familiar with how TAV works but when it installs different versions of redis, it only passes if no single version is specified in dependencies. I would like the owner to review if possible.
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.
Curious why this is the odd package out in needing this.
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.
In cjs projects, mocha isn't able to process .mjs files without this config.
GCP has this unusual fixture:
argv: ['fixtures/detect-with-http-instrumentation.mjs'],
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.
mocha shouldn't need to process that file, because it is a "fixture": There is a *.test.ts file in this package the exec's that fixture in a subprocess. Similar thing in a number of other packages' test suites.
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.
Ok. Let me run it without the config and I'll post the error here.
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.
Working now. Prob another artifact from TS 5.9
...s-sdk/test/mock-responses/bedrock-invokemodel-adds-amazon-nova-model-attributes-to-span.json
Show resolved
Hide resolved
4797803 to
723401a
Compare
723401a to
3056842
Compare
|
@trentm If Node 18 support is needed then there's more work to do. I missed this requirement when I started this work. 😕 How do you want to proceed? Try to roll back dependency versions until it's happy? ❯ npm ci
npm warn EBADENGINE Unsupported engine {
npm warn EBADENGINE package: '@azure-rest/[email protected]',
npm warn EBADENGINE required: { node: '>=20.0.0' },
npm warn EBADENGINE current: { node: 'v18.20.8', npm: '10.8.2' }
npm warn EBADENGINE }
npm warn EBADENGINE Unsupported engine {
npm warn EBADENGINE package: '@azure/[email protected]',
npm warn EBADENGINE required: { node: '>=20.0.0' },
npm warn EBADENGINE current: { node: 'v18.20.8', npm: '10.8.2' }
npm warn EBADENGINE } |
3056842 to
804aa71
Compare
🤔 We say we support Active and Maintenance LTS versions of Node, and Node 18 is officially end of life. So it might be fine in Contrib to drop support for 18. That's a bigger discussion though (cc @open-telemetry/javascript-maintainers ) and we'll need to update docs too. |
@overbalance I don't think those EBADENGINE warnings are blocking for this PR. We already have a number of those warnings in the current state. Unfortunately an EBADENGINE warning will matter for some deps, but not for some deps that are just for testing. For example this one: That dep is used by tedious@17: which is a test dep. We intentionally install a recent-ish major version of |
46672a0 to
6ac6dbd
Compare
|
@trentm Those warnings are blocking CI from what I see. What do you recommend? |
I don't know what is going on. I can This CI step failed with a network error: https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/18537223726/job/53057847496?pr=3032 The other test jobs are stuck in The Ideas on what this could be:
|
4c414bc to
99cc06d
Compare
99cc06d to
324495b
Compare
What this does
Hoists all build devDependencies to the root package.json.
Key changes
Test fixes
Dependencies
Configuration
Package-specific changes
@opentelemetry/instrumentation-socket.io
import * as expecttoimport expect(expect v29 uses default export)@opentelemetry/instrumentation-dns
@opentelemetry/instrumentation-aws-sdk
Browser packages (propagator-aws-xray, propagator-instana, instrumentation-user-interaction, instrumentation-long-task)