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.

Sign up for GitHub

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: moving dev dependency to main dependency due to upgrade to ts 4 #460

Closed
wants to merge 1 commit into from
Closed

chore: moving dev dependency to main dependency due to upgrade to ts 4 #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-aws-lambda/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
},
"devDependencies": {
"@opentelemetry/node": "0.19.0",
"@types/aws-lambda": "8.10.76",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"codecov": "3.8.1",
Expand All @@ -60,6 +59,7 @@
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/resources": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0",
"@opentelemetry/tracing": "^0.19.0"
"@opentelemetry/tracing": "^0.19.0",
"@types/aws-lambda": "8.10.76"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we don't pin dependencies, only dev-dependencies

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved them to different location, don't want to change them in any other way

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we actually do pin all deps that aren't @opentelemetry deps usually iirc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only dependency I found is semver e.g. here which is not pinned.

We should be careful which version we use for type definitions. If npm is not able to de-duplicate them users end up in having two versions of e.g. @types/aws-lambda in the same project which usually not what someone wants.

I doubt that typescript is able to use version A for one file and version B for other files. And clearly types will differ between versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to think about using * if that is the case.

In this case, I think it's probably ok since we don't "leak" any of the types from the packages we wrap. If we use @types/pg@1 and the user has @types/pg@2, ours will simply be installed in the deep node_modules directory at something like node_modules/@opentelemetry/instrumentation-pg/node_modules/@types/pg no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to tell without trying.
Main question is what is typescript using - and remembering - when transpiling a file like this and the user has @types/pg installed in a significant different version then @opentelemetry/instrumentation-pg.

import * from "pg"
import * from "@opentelemetry/instrumentation-pg"

I assume as long as the only types used are protected init(): (InstrumentationNodeModuleDefinition<typeof pgTypes> | InstrumentationNodeModuleDefinition<typeof pgPoolTypes>)[]; in an protected method never used by end users it's don't care.

Once there are APIs using more types it may get problematic.

maybe we should somehow hide/remove the types from InstrumentationAbstract#init to avoid this at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guys you are aware that because we upgraded to ts 4 now all the packages doesn't contain the types and if you don't install the package by yrself it cannot be used. This change is just fixing to how it was, should be already merged and released. Then we can discuss about all other things but currently the packages are broken especially the meta package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obecny is right. this simple program fails to compile because of missing types currently:

import type {} from '@opentelemetry/auto-instrumentations-node';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple local test seems to show that typescript correctly differentiates the types. I manually installed @types/pg under node_modules/@opentelemetry/instrumentation-pg/node_modules/@types/pg and installed it also at node_modules/@types/pg. I made a breaking incompatible change to the types manually to the node_modules/@types/pg. My program still compiled successfuly and didn't break the checks for node_modules/@opentelemetry/instrumentation-pg so I think @Flarna's version concerns are OK.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would that be a different if you keep types in dev and compile using ts 3.x ? you would still break it exactly the same ?. The files will be in the same location if you either keep in dev with ts3 or move it to main packages and compile with ver. 4. You will have exactly the same pinned version of types right ?

}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-bunyan/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
"@opentelemetry/context-async-hooks": "0.19.0",
"@opentelemetry/node": "0.19.0",
"@opentelemetry/tracing": "0.19.0",
"@types/bunyan": "1.8.6",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"@types/sinon": "9.0.11",
Expand All @@ -63,6 +62,7 @@
},
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0"
"@opentelemetry/instrumentation": "^0.19.0",
"@types/bunyan": "1.8.6"
}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-express/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
"@opentelemetry/context-async-hooks": "0.19.0",
"@opentelemetry/node": "0.19.0",
"@opentelemetry/tracing": "0.19.0",
"@types/express": "4.17.11",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"codecov": "3.8.1",
Expand All @@ -63,6 +62,7 @@
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/core": "^0.19.0",
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0"
"@opentelemetry/semantic-conventions": "^0.19.0",
"@types/express": "4.17.11"
}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-graphql/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
},
"devDependencies": {
"@opentelemetry/tracing": "0.19.0",
"@types/graphql": "14.5.0",
"@types/mocha": "8.2.2",
"@types/node": "14.14.41",
"codecov": "3.8.1",
Expand All @@ -58,6 +57,7 @@
},
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0"
"@opentelemetry/instrumentation": "^0.19.0",
"@types/graphql": "14.5.0"
}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-hapi/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
"@opentelemetry/context-async-hooks": "0.19.0",
"@opentelemetry/node": "0.19.0",
"@opentelemetry/tracing": "0.19.0",
"@types/hapi__hapi": "20.0.8",
"@types/mocha": "7.0.2",
"@types/node": "12.20.10",
"codecov": "3.8.1",
Expand All @@ -62,6 +61,7 @@
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0"
"@opentelemetry/semantic-conventions": "^0.19.0",
"@types/hapi__hapi": "20.0.8"
}
}
5 changes: 2 additions & 3 deletions plugins/node/opentelemetry-instrumentation-ioredis/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,8 @@
"@opentelemetry/node": "0.19.0",
"@opentelemetry/test-utils": "^0.16.0",
"@opentelemetry/tracing": "0.19.0",
"@types/ioredis": "4.26.0",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"@types/shimmer": "1.0.1",
"codecov": "3.8.1",
"cross-env": "7.0.3",
"gts": "3.1.0",
Expand All @@ -67,6 +65,7 @@
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0"
"@opentelemetry/semantic-conventions": "^0.19.0",
"@types/ioredis": "4.26.0"
}
}
6 changes: 3 additions & 3 deletions plugins/node/opentelemetry-instrumentation-koa/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@
"@opentelemetry/context-async-hooks": "0.19.0",
"@opentelemetry/node": "0.19.0",
"@opentelemetry/tracing": "0.19.0",
"@types/koa": "2.13.1",
"@types/koa__router": "8.0.4",
"@types/mocha": "7.0.2",
"@types/node": "12.20.10",
"codecov": "3.8.1",
Expand All @@ -65,6 +63,8 @@
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0"
"@opentelemetry/semantic-conventions": "^0.19.0",
"@types/koa": "2.13.1",
"@types/koa__router": "8.0.4"
}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-mongodb/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
"@opentelemetry/node": "0.19.0",
"@opentelemetry/tracing": "0.19.0",
"@types/mocha": "7.0.2",
"@types/mongodb": "3.6.12",
"@types/node": "14.14.41",
"codecov": "3.8.1",
"gts": "3.1.0",
Expand All @@ -63,6 +62,7 @@
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0"
"@opentelemetry/semantic-conventions": "^0.19.0",
"@types/mongodb": "3.6.12"
}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-mysql/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
"@opentelemetry/test-utils": "^0.16.0",
"@opentelemetry/tracing": "0.19.0",
"@types/mocha": "7.0.2",
"@types/mysql": "2.15.18",
"@types/node": "14.14.41",
"codecov": "3.8.1",
"gts": "3.1.0",
Expand All @@ -61,6 +60,7 @@
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0"
"@opentelemetry/semantic-conventions": "^0.19.0",
"@types/mysql": "2.15.18"
}
}
6 changes: 3 additions & 3 deletions plugins/node/opentelemetry-instrumentation-pg/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@
"@opentelemetry/tracing": "0.19.0",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"@types/pg": "7.14.11",
"@types/pg-pool": "2.0.2",
"codecov": "3.8.1",
"cross-env": "7.0.3",
"gts": "3.1.0",
Expand All @@ -70,6 +68,8 @@
},
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0"
"@opentelemetry/instrumentation": "^0.19.0",
"@types/pg": "7.14.11",
"@types/pg-pool": "2.0.2"
}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-pino/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
"@opentelemetry/tracing": "0.19.0",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"@types/pino": "6.3.8",
"@types/semver": "7.3.5",
"@types/sinon": "9.0.11",
"codecov": "3.8.1",
Expand All @@ -66,6 +65,7 @@
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0",
"semver": "^7.3.5"
"semver": "^7.3.5",
"@types/pino": "6.3.8"
}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-redis/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
"@opentelemetry/tracing": "0.19.0",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"@types/redis": "2.8.28",
"codecov": "3.8.1",
"cross-env": "7.0.3",
"gts": "3.1.0",
Expand All @@ -65,6 +64,7 @@
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0"
"@opentelemetry/semantic-conventions": "^0.19.0",
"@types/redis": "2.8.28"
}
}
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-restify/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
"@opentelemetry/tracing": "0.19.0",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"@types/restify": "4.3.7",
"codecov": "3.8.1",
"gts": "3.1.0",
"mocha": "7.2.0",
Expand All @@ -61,6 +60,7 @@
"dependencies": {
"@opentelemetry/api": "^1.0.0-rc.0",
"@opentelemetry/instrumentation": "^0.19.0",
"@opentelemetry/semantic-conventions": "^0.19.0"
"@opentelemetry/semantic-conventions": "^0.19.0",
"@types/restify": "4.3.7"
}
}
1 change: 0 additions & 1 deletion plugins/web/opentelemetry-instrumentation-user-interaction/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
"@types/jquery": "3.5.5",
"@types/mocha": "7.0.2",
"@types/node": "14.14.41",
"@types/shimmer": "1.0.1",
"@types/sinon": "9.0.11",
"@types/webpack-env": "1.16.0",
"babel-loader": "8.2.2",
Expand Down