chore: add 'lint:deps' script to check for unused and unlisted deps

[trentm]

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.

[trentm]

I wonder if we can specifically exclude by name

Got an answer in commit 47ee30b.

[trentm]

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]

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]

(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.

[github-actions]

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]

(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.)

[trentm]

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?

[blumamir]

Thanks for adding it!

The knip tool looks great and found many important issues.
Running it as npx makes sense to me. Do you think we should replace it to dev dependency in the future when we upgrade typescript to v5?

Could be awesome to enable more of it's features later on and potentially fix more dependency / files / exports issues it will report.

[blumamir]

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?

I think that if we can make it a dev dependency once we upgrade to typescript 5, then it's scoped in time (hopefully short time) and we can skip the documentation. If you think it's important, I would consider maybe having it in the knip configuration file? hoping one will take a look there before making any changes to knip setup

I am also ok with DEVELOPMENT.md if that makes most sense to you

[blumamir]

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.

Do you think it makes sense to add a CI check for that? that will just run npm ci and make sure a PR will not blow up the installation size?
I don't have time to work on it myself at the moment, just sharing thoughts and potential alternatives for someone to pick up or not :)

[trentm]

Do you think it makes sense to add a CI check for that? that will just run npm ci and make sure a PR will not blow up the installation size?

Perhaps, yes. :) If we had some infra for collecting dev stats per commit, that would be nice. We'd either have to set a static size threshold or have it only check in CI and compare against previous values. Also something I won't have time for right now.

[trentm]

I think that if we can make it a dev dependency once we upgrade to typescript 5, then it's scoped in time (hopefully short time) and we can skip the documentation.

I added a note to follow-up in open-telemetry/opentelemetry-js#4870 (comment)