Repository navigation
Detect unused code and dependencies with knip - #1492
Merged
Merged
Conversation
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
added
maintenance
✅ Deploy Preview for testcontainers-node ready!
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
added
minor
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 was referenced Oct 10, 2026
cristianrgreco
added this pull request to stack #1498
October 10, 2026 20:35
cristianrgreco
removed this pull request from stack #1498
October 11, 2026 18:15
A wider search of public code found six repositories subclassing AbstractWaitStrategy through a deep import, with no equivalent in the index, and one importing the Labels type.
# Conflicts: # packages/modules/weaviate/src/weaviate-container.test.ts
The undeclared imports in azurite, k3s and selenium are declared on main, so knip no longer needs to ignore them.
# Conflicts: # .github/actions/npm-setup/action.yml # package.json
# Conflicts: # package-lock.json
main no longer has @chroma-core/default-embed as a dev dependency, so there is nothing for knip to ignore.
cristianrgreco
added
patch
Custom wait strategies are already covered by StartupCheckStrategy and Wait.forAll. The two public codebases that subclass AbstractWaitStrategy don't need it: one runs two built-in strategies in sequence and the other doesn't wait at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds knip to find unused files, exports and dependencies across the workspaces, and fixes what it reports.
knip6.39.0 as a root devDependency, pinned exactly. It's the newest release older than the 7-daymin-release-agein.npmrc.knip.jsoncwith a comment on every entry and ignore.npm run knipscript, run as a second step in the existingLintjob inchecks.yml. That job already installs every workspace, which knip needs: it reads the manifests of installed dependencies, and with only one workspace installed it stops reporting@types/amqplib.AGENTS.md:npm run knipadded 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 frombuild/index.jstosrc/index.ts), Vitest test files and config, and the scripts the workflows run withnode(.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'sindex.tsexports, 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 fortestcontainers/build, the names thatindex.tsdidn't export were:EnvironmentStartedGenericContainerHealthCheckLogWaitStrategyContentToCopy,FileToCopyLoggerBindMode,HttpWaitStrategyThis PR therefore:
Environment,HealthCheck,ContentToCopy,FileToCopy,BindModeandLabelsfromtestcontainers'index.ts. They stay exported fromsrc/types.tstoo, so existing deep imports of them keep working.StartedGenericContainer,AbstractWaitStrategyor the concrete wait strategies (LogWaitStrategy,HttpWaitStrategy,ShellWaitStrategy). The index already covers how they're used:StartedTestContaineras the type, theWaitfactory to create strategies, andStartupCheckStrategyfor custom ones. Their existing deep-import paths are unchanged.A wider search later confirmed this (15 code-search queries, 609 public files):
EnvironmentStartedGenericContainerStartedTestContainerLogWaitStrategynewWait.forLogMessage()AbstractWaitStrategyextendsStartupCheckStrategy,Wait.forAll()HttpWaitStrategynewWait.forHttp()ContentToCopy,FileToCopy,HealthCheck,BindMode,LabelsShellWaitStrategynewWait.forSuccessfulCommand()None of the names this PR removes appears in any result.
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 (13)
docs/site/js/tc-header.js<script>tag indocs/site/theme/main.htmlentrypackages/testcontainers/smoke-test.jest.js--testMatchinchecks.ymlentrypackages/testcontainers/fixtures/**/index.jsignoreFiles@chroma-core/default-embedimport()s it as the default embedding function@google-cloud/firestorefirebase-admin/firestorerequire()s it, but firebase-admin only lists it as an optional dependencyignoreDependenciesAlready fixed by #1485 (5)
Knip independently flags the same undeclared imports as #1485:
tar-stream(k3s),tar-fsandtmp(selenium, in source and test), and@azure/core-auth(azurite test utils). This PR doesn't duplicate that fix. #1485 has since merged, and withmainmerged in here knip no longer reports them, so the temporary ignores this PR carried for them are removed.Real findings (17)
Sixteen are fixed here. The seventeenth,
LABEL_TESTCONTAINERS_LANG, is no longer a finding: #1453 has since merged and imports it inreaper.ts, so it stays exported.@types/amqplibamqplib2.x ships its ownindex.d.ts.@types/properties-readerproperties-reader3.x ships its own types.test-helper.ts:getStoppedContainerNames,getContainerIds,checkImageExists,getRunningNetworkIdstsconfig.build.jsonexcludestest-helper.ts, so it was never published.types.ts:BindModeindex.ts(see above).types.ts:ContainerRuntimecontainer/types.ts:EnvironmentEnvironmentfromtypes.ts.compose/types.ts:ComposeExecutableOptions(and its re-export fromcontainer-runtime/index.ts)exportremoved. Used only in its own file.labels.ts:LABEL_TESTCONTAINERS,LABEL_TESTCONTAINERS_VERSIONexportremoved. Used only bycreateLabels()in the same file.health-check.ts:isHealthCheckDisabled,getHealthCheckConfigexportremoved. Used only in the same file.OLLAMA_PORTexportremoved. Used only in the same file.--productionmode was also tried. It adds only exports that are used by tests (for exampleLOCALSTACK_PORT), so the default mode is used.Lockfile
Besides knip's own dependency tree, which is all
dev, one existing entry moves up a patch version because knip's range requires it:yaml2.9.0 → 2.9.1 (knip needs^2.9.1)The only other lockfile changes are removing
@types/amqpliband@types/properties-reader. There's no unrelated drift.Verification
npm ci: lockfile unchangednpm run knip: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).--treat-config-hints-as-errors.BindModeandContainerRuntimetemporarily exported fromindex.ts, knip stops reporting both, even though nothing in the repository usesContainerRuntime.npm run format: no fixes applied (457 files)npm run lint: no fixes applied (457 files)npm run check-compilesfrom a clean build: passes. The emittedbuild/index.d.tsexports the five new types.npx vitest runonhealth-check.test.ts,wait-strategy-selector.test.ts,test-helper.test.ts,generic-container-auto-cleanup.test.ts,packages/modules/weaviate/srcandpackages/modules/ollama/src(Docker): 6 files, 24 tests passed, 1 skipped (an existingit.skipin the ollama tests)main:npx vitest runonpackages/testcontainers/src/reaper,health-check.test.tsandgeneric-container-auto-cleanup.test.ts(Docker): 3 files, 18 tests passed.generic-container-commit.test.tsandpackages/modules/rabbitmq/srcpassed.main(which now has the singleLintjob and shared cache from Run lint and compile once and share one dependency cache #1495):npm run lint:ci,npm run knipandnpm run check-compilespass, andchecks.ymlparses with knip as a step ofLint.Semver:
patch, not breakingtestcontainers' index. All of them were already importable frombuild/types, so this only gives existing names a supported path. Labelledpatchon that basis.index.tsexported before, and the runtime behaviour of every package.index.ts, listed in the table above. None of them appear in the deep-import sample.devDependencies, so what consumers install doesn't change.exportsmap, so a consumer could have deep-imported one of the removed names, for exampleLABEL_TESTCONTAINERSfromtestcontainers/build/utils/labels. That would now fail. Those paths aren't supported API.