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: add 'lint:deps' script to check for unused and unlisted deps #2477

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Oct 10, 2024

This uses knip (https://knip.dev) to check dependencies, in hopes of avoiding
issues like #2473. This adds 'npm run lint:deps' and adds that to the
existing 'npm run lint'.

This change includes fixes for a handful of unused and unlisted deps.
For now knip is configured to only check 'production' deps. Checking non-prod
deps results in way too many false positives.

before the fixes in this commit

This is the output of knip --dependencies --production before the fixes included in this commit:

Unused dependencies (10)
@opentelemetry/sdk-metrics     packages/opentelemetry-host-metrics/package.json
@opentelemetry/resources       plugins/node/opentelemetry-instrumentation-aws-lambda/package.json
semver                         plugins/node/opentelemetry-instrumentation-dns/package.json
@opentelemetry/sdk-metrics     plugins/node/opentelemetry-instrumentation-mongodb/package.json
@opentelemetry/sdk-trace-base  plugins/web/opentelemetry-instrumentation-document-load/package.json
@opentelemetry/sdk-trace-web   plugins/web/opentelemetry-instrumentation-long-task/package.json
@opentelemetry/context-zone    plugins/web/opentelemetry-plugin-react-load/package.json
@opentelemetry/sdk-trace-base  plugins/web/opentelemetry-plugin-react-load/package.json
@opentelemetry/sdk-trace-web   plugins/web/opentelemetry-plugin-react-load/package.json
Unlisted dependencies (3)
@opentelemetry/otlp-transformer  packages/opentelemetry-test-utils/src/test-fixtures.ts
@opentelemetry/sdk-metrics       packages/opentelemetry-test-utils/src/test-utils.ts
@cucumber/messages               plugins/node/instrumentation-cucumber/src/instrumentation.ts

The cucumber one was about imported types, so would not have broken users of the published package.

This uses knip (https://knip.dev) to check dependencies, in hopes of avoiding
issues like open-telemetry#2473. This adds 'npm run lint:deps' and adds that to the
existing 'npm run lint'.

This change includes fixes for a handful of unused and unlisted deps.
For now knip is configured to only check 'production' deps. Checking non-prod
deps results in way too many false positives.
@trentm
Copy link
Contributor Author

trentm commented Oct 10, 2024

I wonder if we can specifically exclude by name

Got an answer in commit 47ee30b.

@trentm
Copy link
Contributor Author

trentm commented Oct 10, 2024

Ugh, perhaps the following is fine, but it is annoying. The addition of the knip dep at the top-level resulted in its [email protected] dep being placed at the top-level ./node_modules/typescript... and the [email protected] dep of every other package now being separately installed in every other package's own .../node_modules dir. Boo.

@trentm
Copy link
Contributor Author

trentm commented Oct 10, 2024

Ugh, perhaps the following is fine, but it is annoying.

Actually I think this is unacceptable. The result is a MUCH larger working copy:

Before:

$ npm ci
$ du -sh .
1.2G	.

After (i.e. this PR branch):

% du -sh .
4.9G	.

Ew.

@trentm trentm marked this pull request as draft October 10, 2024 22:10
@trentm
Copy link
Contributor Author

trentm commented Oct 10, 2024

(Profanity.)

I don't know how to solve this well. knip has a peerDep on typescript >=5.0.something. I tried to install [email protected] in opentelemetry-js-contrib's top-level ... to force us having the typescript that most packages will use installed at the top-level node_modules/typescript. But the installing knip fails:

% npm install --save-dev knip
npm error code ERESOLVE
npm error ERESOLVE unable to resolve dependency tree
npm error
npm error While resolving: [email protected]
npm error Found: [email protected]
npm error node_modules/typescript
npm error   dev typescript@"^4.4.4" from the root project
npm error
npm error Could not resolve dependency:
npm error peer typescript@">=5.0.4" from [email protected]
npm error node_modules/knip
npm error   dev knip@"*" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.
npm error
npm error
npm error For a full report see:
npm error /Users/trentm/.npm/_logs/2024-10-10T22_06_05_024Z-eresolve-report.txt
npm error A complete log of this run can be found in: /Users/trentm/.npm/_logs/2024-10-10T22_06_05_024Z-debug-0.log

Using --legacy-peer-deps (on the CLI or in .npmrc) isn't a satisfatory answer.

options

  1. Only ever use knip via npx knip .... This installs it separately... somewhere, I'm not exactly sure where. I gather it is outside of the git clone tree, which means it perhaps is not acceptable for developers; it wouild be fine in CI. I think it will prompt if it is okay to install knip. That will be a WTF moment for someone running npm run lint.

  2. Make an esbuild-bundled version of knip and commit that? Or publish it as @trentm/knip or whatever and use that. That isn't very satisfying.

  3. Use pnpm on the (naive) assumption it can help here.

  4. Avoid knip, at least until we've upgrade to typescript@5. (I had tried depcheck earlier and didn't get satisfactory results. I think it missed some things.)

I can't think of other options.

Copy link
Contributor

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.
Are you familiar with this package? Consider becoming a component owner.

@trentm
Copy link
Contributor Author

trentm commented Oct 11, 2024

(Aside: interesting earlier issue related to @cucumber/messages. I'm not sure if having added the @cucumber/messages dep would have solved that older issue.)

After reading more about how npx works
(https://docs.npmjs.com/cli/v8/commands/npm-exec)
I'm more confident that running knip via npx (to workaround the
install-tree issue) is fine for dev and for CI.
@trentm
Copy link
Contributor Author

trentm commented Oct 11, 2024

After reading more about how npx works (https://docs.npmjs.com/cli/v8/commands/npm-exec) I'm more confident that running knip via npx (to workaround the install-tree issue) is fine for dev and for CI.
This is Option (1.) from above.

@open-telemetry/javascript-approvers This PR adds this script to package.json:

"lint:deps": "npx --yes [email protected] --dependencies --production --tags=-knipignore",

I think the usage of npx ... here for running knip -- rather than npm install --save-deving it -- is worth an explanation, so that a future maintainer doesn't naively just npm install it. Do you think this explanation should live in a .md file in the repo somewhere? If so, where? I could start a new DEVELOPMENT.md or a doc/contributing/something.md (similar to what is in the core repo). Or we could just leave it to this PR (I'd update the PR description with notes). Thoughts?

@trentm trentm marked this pull request as ready for review October 11, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:host-metrics pkg:instrumentation-amqplib pkg:instrumentation-aws-lambda pkg:instrumentation-aws-sdk pkg:instrumentation-bunyan pkg:instrumentation-cassandra-driver pkg:instrumentation-connect pkg:instrumentation-cucumber pkg:instrumentation-dataloader pkg:instrumentation-dns pkg:instrumentation-document-load pkg:instrumentation-express pkg:instrumentation-fastify pkg:instrumentation-fs pkg:instrumentation-generic-pool pkg:instrumentation-graphql pkg:instrumentation-hapi pkg:instrumentation-ioredis pkg:instrumentation-knex pkg:instrumentation-koa pkg:instrumentation-long-task pkg:instrumentation-lru-memoizer pkg:instrumentation-memcached pkg:instrumentation-mongodb pkg:instrumentation-mongoose pkg:instrumentation-mysql pkg:instrumentation-mysql2 pkg:instrumentation-nestjs-core pkg:instrumentation-net pkg:instrumentation-pg pkg:instrumentation-pino pkg:instrumentation-redis pkg:instrumentation-redis-4 pkg:instrumentation-restify pkg:instrumentation-router pkg:instrumentation-runtime-node pkg:instrumentation-socket.io pkg:instrumentation-tedious pkg:instrumentation-undici pkg:instrumentation-user-interaction pkg:instrumentation-winston pkg:plugin-react-load pkg:propagation-utils pkg:resource-detector-alibaba-cloud pkg:resource-detector-aws pkg:resource-detector-azure pkg:resource-detector-container pkg:resource-detector-gcp pkg:resource-detector-instana pkg:sampler-aws-xray pkg:test-utils pkg-status:unmaintained:autoclose-scheduled pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants