chore: setup knip#4893
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb60c2b210
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // injected by bundler, so it can not use runtime-core stuff | ||
| export function getBuilderId(): string { | ||
| function getBuilderId(): string { |
There was a problem hiding this comment.
For consumers using the package's wildcard subpath exports, this turns a previously exported helper into a private function. @module-federation/runtime exposes ./* in its package exports and this module is pulled into the published build from src/index.ts, so imports of the deep utility module that use getBuilderId will fail after upgrading even though Knip cannot see those external references. Keep the export or make the removal an explicit breaking API change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked against the published artifact: dist/utils.js in [email protected] only exports getGlobalFederationInstance. tsdown prunes module exports to what's reachable from the bundle entries, and getBuilderId never was — so a deep import of it already fails on the current release.
Verified by diffing this branch's build against the npm tarball: dist/utils.js is byte-identical to 2.7.0. Dropping the export keyword changes nothing in the published surface.
| } | ||
|
|
||
| export { log, fileLog, error, logger }; | ||
| export { fileLog, error, logger }; |
There was a problem hiding this comment.
This drops log from the dts-plugin server logging module, but @module-federation/dts-plugin publishes dist/ and has wildcard subpath exports, while src/server/index.ts re-exports ./utils/log. Any dev tooling importing the simple logger from the generated server utilities will now get a missing export; external package users are exactly the references Knip will not observe. Keep the export or treat this as a breaking change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There's no stable path that ever exposed log. The published dts-plugin dist contains only the bundle entries (index.js, core.js, the fork workers, the runtime plugin) plus content-hashed chunks (utils-7KqCZHbb.js etc.) — src/server/utils/log.ts doesn't exist as an addressable module, and none of the entries re-export it. The ./* wildcard only resolves to files that exist in the tarball, and a hashed chunk path breaks on every release anyway.
The emitted dist/*.d.ts are byte-identical before and after this change, so the importable surface is provably unchanged.
|
Zero-config knip reported 49 issues for dts-plugin; most were false positives from runtime-loaded build entries (forked workers, the runtime plugin) and test-runtime tooling (vue-tsc fixture). The workspace config declares those as entries/ignores so only real findings remain.
None of these were reachable from the package's published entry points: two orphaned modules, an unused rimraf devDependency, exports only referenced by internal barrels, and broker protocol enum members nothing ever sends. The emitted dist/*.d.ts are byte-identical before and after, so the public API surface is untouched.
The sdk needs no knip config beyond registering the workspace; its one finding, an orphaned test fixture, is deleted here.
Fixes the findings: two test mocks still imported src/global and src/type, which moved to runtime-core in the module-federation#3154 split, and getBuilderId only needed the export keyword dropped since its sole caller lives in the same file.
Fixes the findings: drops a dead sdk re-export, isDevelopmentMode and isLegallyVersion which nothing calls, and export keywords on helpers only used inside their own module. The duplicates rule is off (knip only supports this at root level) since Global is an intentional public alias of nativeGlobal.
The typescript-7 npm alias from module-federation#4878 is resolved by string in tests, which knip cannot trace, so it joins the ignore list. The two resolver helpers it flagged landed without any callers and are dropped.
|
@2heal1 is there any appetite in introducing knip to continuously prevent stale code paths? If not I'm going to close this. |
1 participant
Description
During my recent dependency sweep I noticed the repository contains dead code in a few places. I'd like to suggest to setup knip to continuously monitor for dead code, unused dependencies ...
You can review the setup per commit - each package one commit. If you agree you'd like to introduce it, I can go through the other packages and set it up for each of them.
In the end we could also setup a GitHub action that runs against each PR, but before doing so, I'd like the maintainers input, whether its a welcome contribution.
Related Issue
Types of changes
Checklist