Skip to content

Detect unused code and dependencies with knip - #1492

Draft
cristianrgreco wants to merge 9 commits into
mainfrom
claude/knip-5dccc1
Draft

cristianrgreco wants to merge 9 commits into
mainfrom
claude/knip-5dccc1

Conversation

@cristianrgreco

@cristianrgreco cristianrgreco commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Adds knip to find unused files, exports and dependencies across the workspaces, and fixes what it reports.

  • knip 6.39.0 as a root devDependency, pinned exactly. It's the newest release older than the 7-day min-release-age in .npmrc.
  • knip.jsonc with a comment on every entry and ignore.
  • npm run knip script, and a single Knip job in checks.yml that the Checks complete gate depends on. Knip analyses all workspaces together, so one job fits better than the per-module matrix. It has no if, so it also runs when only knip.jsonc changes, which changed-modules.mjs doesn't map to any package.
  • npm-setup action: workspace is now optional. Left empty, it installs every workspace and caches the result under an all-workspaces key. The Knip job uses this. Existing callers and their cache keys are unchanged.
    • Knip needs the full install because it reads the manifests of installed dependencies. With only the testcontainers workspace installed, it no longer reports @types/amqplib, since it can't see that amqplib ships its own types.
    • The full-install cache is 449 MB on CI, against 340 MB for the largest existing per-workspace cache. The repository is already at the 10 GB Actions cache limit, so this adds one more entry to the eviction churn.
    • On this PR the first run missed and saved the cache (npm ci took 35s, job 49s). The next run restored it (install step 11s, job 19s).
  • AGENTS.md: npm run knip added to the required checks, plus a note on what knip treats as public API.

Knip's built-in plugins already find most entry points: each package's main (mapped from build/index.js to src/index.ts), Vitest test files and config, and the scripts the workflows run with node (.github/scripts/changed-modules.mjs, smoke-test.js, smoke-test.mjs). The config only adds what they can't see.

Public API

The supported way to use these packages is to import from the package itself, for example import { GenericContainer } from "testcontainers". Knip follows the same rule: it never reports what a package's index.ts exports, and it reports any other export that no other file uses.

Some public projects deep-import from testcontainers/build/... because the type they need isn't exported from the index. In the first 100 public code-search hits for testcontainers/build, the names that index.ts didn't export were:

Name Hits
Environment 20
StartedGenericContainer 10
HealthCheck 5
LogWaitStrategy 4
ContentToCopy, FileToCopy 3
Logger 2
BindMode, HttpWaitStrategy 1 each

This PR therefore:

  • Exports Environment, HealthCheck, ContentToCopy, FileToCopy and BindMode from testcontainers' index.ts. They stay exported from src/types.ts too, so existing deep imports of them keep working.
  • Switches the one deep import inside this repository (weaviate-container.test.ts) to import type { Environment } from "testcontainers".
  • Doesn't export the classes (StartedGenericContainer, LogWaitStrategy, HttpWaitStrategy, Logger). That's left as a separate decision, since StartedTestContainer, Wait and log may already cover those uses from the index. Their existing deep-import paths are unchanged.

Findings

Knip with an empty config reported 35 issues: 11 unused files, 4 unused devDependencies, 5 unlisted dependencies, 11 unused exports and 4 unused exported types.

False positives, handled in knip.jsonc (13)

Finding Why it's a false positive Config
docs/site/js/tc-header.js Loaded by a <script> tag in docs/site/theme/main.html entry
packages/testcontainers/smoke-test.jest.js Run by Jest through --testMatch in checks.yml entry
9 × packages/testcontainers/fixtures/**/index.js Docker build contexts, never imported ignoreFiles
chromadb: @chroma-core/default-embed chromadb import()s it as the default embedding function ignoreDependencies
gcloud: @google-cloud/firestore firebase-admin/firestore require()s it, but firebase-admin only lists it as an optional dependency ignoreDependencies

Already fixed by #1485 (5)

Knip independently flags the same undeclared imports as #1485: tar-stream (k3s), tar-fs and tmp (selenium, in source and test), and @azure/core-auth (azurite test utils). This PR doesn't duplicate that fix. It ignores the five imports in knip.jsonc under a TODO so knip passes on its own. Whichever PR merges second should remove those three workspace entries. If they're left in, knip prints a configuration hint but still passes.

Real findings (17)

Sixteen are fixed here. The seventeenth, LABEL_TESTCONTAINERS_LANG, is no longer a finding: #1453 has since merged and imports it in reaper.ts, so it stays exported.

Finding Fix
rabbitmq: @types/amqplib Removed. amqplib 2.x ships its own index.d.ts.
testcontainers: @types/properties-reader Removed. properties-reader 3.x ships its own types.
test-helper.ts: getStoppedContainerNames, getContainerIds, checkImageExists, getRunningNetworkIds Deleted. Nothing calls them, and tsconfig.build.json excludes test-helper.ts, so it was never published.
types.ts: BindMode Now exported from index.ts (see above).
types.ts: ContainerRuntime Deleted. Nothing uses it.
container/types.ts: Environment Duplicate removed. The file now imports the shared Environment from types.ts.
compose/types.ts: ComposeExecutableOptions (and its re-export from container-runtime/index.ts) export removed. Used only in its own file.
labels.ts: LABEL_TESTCONTAINERS, LABEL_TESTCONTAINERS_VERSION export removed. Used only by createLabels() in the same file.
health-check.ts: isHealthCheckDisabled, getHealthCheckConfig export removed. Used only in the same file.
ollama: OLLAMA_PORT export removed. Used only in the same file.

--production mode was also tried. It adds only exports that are used by tests (for example LOCALSTACK_PORT), so the default mode is used.

Lockfile

Besides knip's own dependency tree, which is all dev, two existing entries move up a patch version because knip's ranges require it:

  • yaml 2.9.0 → 2.9.1 (knip needs ^2.9.1)
  • @emnapi/runtime 1.11.1 → 1.11.2 (pinned exactly by @oxc-resolver/binding-wasm32-wasi)

The only other lockfile changes are removing @types/amqplib and @types/properties-reader. There's no unrelated drift.

Verification

  • npm ci: lockfile unchanged
  • npm run knip:
    • Red: with this config and main's sources as of when this PR was opened, knip exits 1 and reports the 17 real findings (2 devDependencies, 11 exports, 4 types).
    • Green: on this branch it exits 0 with no issues and no configuration hints. It also passes with --treat-config-hints-as-errors.
    • Entry exports: with BindMode and ContainerRuntime temporarily exported from index.ts, knip stops reporting both, even though nothing in the repository uses ContainerRuntime.
  • npm run format: no fixes applied (457 files)
  • npm run lint: no fixes applied (457 files)
  • npm run check-compiles from a clean build: passes. The emitted build/index.d.ts exports the five new types.
  • npx vitest run on health-check.test.ts, wait-strategy-selector.test.ts, test-helper.test.ts, generic-container-auto-cleanup.test.ts, packages/modules/weaviate/src and packages/modules/ollama/src (Docker): 6 files, 24 tests passed, 1 skipped (an existing it.skip in the ollama tests)
  • After merging main: npx vitest run on packages/testcontainers/src/reaper, health-check.test.ts and generic-container-auto-cleanup.test.ts (Docker): 3 files, 18 tests passed.
  • Earlier in this PR, before the export changes: generic-container-commit.test.ts and packages/modules/rabbitmq/src passed.
  • On CI, the Knip job passed using the new full-install mode of npm-setup.

Semver: minor, not breaking

  • Added: five types exported from testcontainers' index. This is new public API, so minor.
  • Unchanged: everything index.ts exported before, and the runtime behaviour of every package.
  • Removed, but never public: 8 exports and 1 unused type in files other than index.ts, listed in the table above. None of them appear in the deep-import sample.
  • The removed packages are devDependencies, so what consumers install doesn't change.
  • Caveat: the packages have no exports map, so a consumer could have deep-imported one of the removed names, for example LABEL_TESTCONTAINERS from testcontainers/build/utils/labels. That would now fail. Those paths aren't supported API.

Follow-ups

Three CI changes are stacked on this branch as separate PRs: one whole-repository Lint job (Biome, then knip), one whole-repository Compile job, and one shared dependency cache.

Add knip with a documented config, an npm script and a Checks job, and
fix what it reports: remove two @types packages whose libraries now ship
their own types, delete unused test helpers and an unused type, and drop
export from internal symbols only used in their own file. Nothing
exported from a package's index.ts changes.
@cristianrgreco cristianrgreco added maintenance Improvements that do not change functionality patch Backward compatible bug fix labels Oct 9, 2026
@netlify

netlify Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit db15187
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-node/deploys/6ac8bcf3f1b1f000084fd0fc
😎 Deploy Preview https://deploy-preview-1492--testcontainers-node.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

The packages have no `exports` map, so files under `build/` are reachable
and removing an `export` there can break consumers. Restore the exports
and configure knip to only report exports that aren't used anywhere.
Make the action's `workspace` input optional. Left empty, it installs
every workspace and caches the result under an `all-workspaces` key.
Knip needs the full install because it reads the manifests of installed
dependencies.
Consumers deep-import Environment, HealthCheck, ContentToCopy, FileToCopy
and BindMode from testcontainers/build/types because the index doesn't
export them. Export them from index.ts so importing from "testcontainers"
is enough.

With that in place, let knip report every export outside index.ts that no
other file uses, and remove the ones it finds.
@cristianrgreco cristianrgreco added minor Backward compatible functionality and removed patch Backward compatible bug fix labels Oct 10, 2026
main now imports it in reaper.ts (#1453), so it's no longer only used in
its own file.
The container client declared its own copy of Environment. Import the one
from types.ts instead.
Biome checks every file in well under a second, so the per-module lint
matrix mostly paid for runner start-up and installs. Replace it and the
separate Knip job with one Lint job that runs Biome and then knip.
This reverts 048b26f here. The change is proposed separately, stacked on
this branch.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Improvements that do not change functionality minor Backward compatible functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant