refactor: remove unused code/deps, configure knip - #7117
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR integrates Knip for monorepo unused-code detection. It adds configuration, workspace rules, a root script, pull request CI, and a skill guide. It removes unused exports, types, logging helpers, parser logic, and package development dependencies. It removes JavaScript bundle format support and updates related schemas and fixtures. It also updates build-info test-server wiring and the conventional-commit workflow. Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
serhalp
commented
Jun 26, 2026
| "@vitest/ui": "^3.0.0", | ||
| "execa": "^8.0.0", | ||
| "memfs": "^4.0.0", | ||
| "typescript": "^5.0.0", |
Member
Author
There was a problem hiding this comment.
typescript was assumed to be hoisted in the root but was declared instead as a dev dep in many (but not all) individual packages. I centralized it as a dev dep in the root.
serhalp
commented
Jun 26, 2026
| "execa": "^8.0.0", | ||
| "memfs": "^4.0.0", | ||
| "typescript": "^5.0.0", | ||
| "ts-node": "^10.9.2", |
Member
Author
There was a problem hiding this comment.
ts-node was missing from this package; it just happened to work due to hoisting
Contributor
|
| 📦 Package | 📏 Size |
|---|---|
| @napi-rs/wasm-runtime@1.1.6 | 6.3 MB |
| @oxc-parser/binding-linux-x64-gnu@0.137.0 | 2.4 MB |
| @oxc-resolver/binding-linux-x64-gnu@11.21.3 | 2.4 MB |
| @emnapi/core@1.11.1 | 2 MB |
| @emnapi/core@1.11.0 | 2 MB |
| knip@6.21.0 | 1.7 MB |
| oxc-parser@0.137.0 | 1.4 MB |
| @tybys/wasm-util@0.10.3 | 825.1 kB |
| @emnapi/runtime@1.11.1 | 433.5 kB |
| @emnapi/runtime@1.11.0 | 433.5 kB |
| @emnapi/wasi-threads@1.2.2 | 225.1 kB |
| unbash@4.0.1 | 180.9 kB |
| import-meta-resolve@4.2.0 | -79.2 kB |
| @commitlint/rules@19.8.1 | -70.6 kB |
| oxc-resolver@11.21.3 | 60.2 kB |
| lodash.mergewith@4.6.2 | -54.2 kB |
| jiti@2.6.1 → jiti@2.7.0 | 50.5 kB |
| @oxc-project/types@0.137.0 | 44.5 kB |
| @commitlint/load@19.8.1 | -36.3 kB |
| @commitlint/cli@19.8.1 | -32.5 kB |
| conventional-commits-parser@5.0.0 | -29.9 kB |
| tar@7.5.16 → tar@7.5.19 | 23.2 kB |
| tinyexec@1.2.4 | -22.9 kB |
| formatly@0.3.0 | 21.6 kB |
| split2@4.2.0 | -17.4 kB |
| @commitlint/resolve-extends@19.8.1 | -17 kB |
| @commitlint/lint@19.8.1 | -14 kB |
| cosmiconfig-typescript-loader@6.3.0 | -13.9 kB |
| @commitlint/read@19.8.1 | -13.2 kB |
| ini@4.1.1 | -12.7 kB |
| dargs@8.1.0 | -12.3 kB |
| @commitlint/config-validator@19.8.1 | -12.1 kB |
| @commitlint/ensure@19.8.1 | -12 kB |
| @commitlint/format@19.8.1 | -10.5 kB |
| strip-json-comments@5.0.3 | 8.2 kB |
| global-directory@4.0.1 | -8.1 kB |
| git-raw-commits@4.0.0 | -7.9 kB |
| fd-package-json@2.0.0 | 7.7 kB |
| @commitlint/is-ignored@19.8.1 | -6.9 kB |
| text-extensions@2.4.0 | -6.3 kB |
| @commitlint/parse@19.8.1 | -4.6 kB |
| @commitlint/execute-rule@19.8.1 | -3.9 kB |
| @commitlint/top-level@19.8.1 | -3.7 kB |
| is-text-path@2.0.0 | -2.9 kB |
| @types/is-ci@3.0.4 | -2.9 kB |
| @commitlint/to-lines@19.8.1 | -2.7 kB |
| @commitlint/message@19.8.1 | -2.7 kB |
| js-yaml@3.14.2 → js-yaml@3.15.0 | 1.2 kB |
| p-map@7.0.4 → p-map@7.0.5 | 490 B |
| nock@14.0.15 → nock@14.0.16 | 393 B |
| @typescript-eslint/types@8.62.0 → @typescript-eslint/types@8.62.1 | 0 B |
| @typescript-eslint/visitor-keys@8.62.0 → @typescript-eslint/visitor-keys@8.62.1 | 0 B |
| @typescript-eslint/project-service@8.62.0 → @typescript-eslint/project-service@8.62.1 | 0 B |
| @typescript-eslint/tsconfig-utils@8.62.0 → @typescript-eslint/tsconfig-utils@8.62.1 | 0 B |
| @typescript-eslint/typescript-estree@8.62.0 → @typescript-eslint/typescript-estree@8.62.1 | 0 B |
| lodash.isplainobject@4.0.6 | Unknown |
| lodash.kebabcase@4.1.1 | Unknown |
| lodash.snakecase@4.1.1 | Unknown |
| lodash.startcase@4.4.0 | Unknown |
| lodash.uniq@4.5.0 | Unknown |
| lodash.upperfirst@4.3.1 | Unknown |
Total size change: 19.9 MB
serhalp
commented
Jun 26, 2026
| "@types/lodash-es": "^4.17.6", | ||
| "@types/node": "^22.12.0", | ||
| "ava": "^5.0.0", | ||
| "c8": "^10.0.0", |
Member
Author
There was a problem hiding this comment.
c8 wasn't used in this package
serhalp
commented
Jun 26, 2026
Comment on lines
-49
to
-50
| "@commitlint/cli": "^19.0.0", | ||
| "@commitlint/config-conventional": "^19.0.0", |
serhalp
commented
Jun 26, 2026
| "@commitlint/config-conventional": "^19.0.0", | ||
| "@eslint/compat": "^1.2.9", | ||
| "@eslint/js": "^9.28.0", | ||
| "@typescript-eslint/eslint-plugin": "~8.49.0", |
Member
Author
There was a problem hiding this comment.
not needed when using typescript-eslint
serhalp
force-pushed
the
serhalp/refactor/remove-unused-and-configure-knip
branch
from
June 26, 2026 22:56
d601446 to
dd29887
Compare
Contributor
|
This pull request adds or modifies JavaScript ( |
serhalp
force-pushed
the
serhalp/refactor/remove-unused-and-configure-knip
branch
from
June 29, 2026 20:46
b3bd4e2 to
8b3c43a
Compare
- c8 wasn't used in that package - typescript was assumed to be hoisted in the root but was declared instead as a dev dep in many (but not all) individual packages - ts-node was missing from one package, just happened to work due to hoisting
This was left over from the deletion in a00e0ea.
serhalp
force-pushed
the
serhalp/refactor/remove-unused-and-configure-knip
branch
from
June 30, 2026 11:15
8b3c43a to
7b78ce3
Compare
serhalp
commented
Jun 30, 2026
| @@ -1,6 +1,5 @@ | |||
| export enum BundleFormat { | |||
| ESZIP2 = 'eszip2', | |||
| JS = 'js', | |||
Member
Author
There was a problem hiding this comment.
Leftover deletion from 2022 removal in a00e0ea
serhalp
force-pushed
the
serhalp/refactor/remove-unused-and-configure-knip
branch
from
June 30, 2026 11:24
54f81b5 to
f496251
Compare
serhalp
marked this pull request as ready for review
June 30, 2026 13:11
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/edge-bundler/node/validation/manifest/index.test.ts (1)
14-15: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression case for the removed
jsformat.The valid fixture now covers
eszip2, but the invalid-format test still uses arbitraryfoo; it would not fail ifjswere accidentally allowed again.Suggested test addition
describe('bundle', () => { + test('should throw on removed js format', () => { + const manifest = getBaseManifest() + manifest.bundles[0].asset = 'function.js' + manifest.bundles[0].format = 'js' + + expect(() => validateManifest(manifest)).toThrowErrorMatchingSnapshot() + }) + test('should throw on additional property in bundle', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/edge-bundler/node/validation/manifest/index.test.ts` around lines 14 - 15, The manifest validation tests need a regression case for the removed js format, since the current invalid-format coverage only checks an arbitrary value and would miss js being reallowed. Update the manifest validation test in index.test.ts to explicitly assert that js is rejected, alongside the existing eszip2 fixture coverage, using the manifest validation path exercised by the current test cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/knip/SKILL.md:
- Around line 14-18: The knip skill guide is using a bare npx-based availability
check even though the repo already provides a lockfile-backed knip dependency
and npm script. Update the setup and command examples in SKILL.md to use the
local script path via npm run knip -- ... (or the repo’s equivalent script), and
remove the npx knip --version check so the documented workflow stays pinned to
the repo-local installation.
In @.github/workflows/conventional-commit.yml:
- Around line 10-12: The conventional-commit workflow currently runs npm ci
before ensuring a specific Node version is installed, so the job depends on
whatever Node/npm is preinstalled on the runner. Update the job to add
actions/setup-node before the Install Dependencies step, using the same Node
version/pinning approach as the other workflows in this repo, and keep the
existing actions/checkout step followed by setup-node and then npm ci.
In @.github/workflows/knip.yml:
- Around line 14-16: The Knip workflow’s checkout step is leaving credentials
persisted by default, which keeps the GitHub token in local git config during
pull_request builds. Update the existing actions/checkout step in the knip job
to disable credential persistence by setting persist-credentials to false. Keep
the change scoped to the checkout configuration so the rest of the workflow
behavior stays the same.
---
Nitpick comments:
In `@packages/edge-bundler/node/validation/manifest/index.test.ts`:
- Around line 14-15: The manifest validation tests need a regression case for
the removed js format, since the current invalid-format coverage only checks an
arbitrary value and would miss js being reallowed. Update the manifest
validation test in index.test.ts to explicitly assert that js is rejected,
alongside the existing eszip2 fixture coverage, using the manifest validation
path exercised by the current test cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d74fc97e-8f6d-4382-ad83-23d7b5a543c0
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/edge-bundler/node/validation/manifest/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (40)
.agents/skills/knip/SKILL.md.claude/skills/knip.github/workflows/conventional-commit.yml.github/workflows/knip.ymlknip.jsoncpackage.jsonpackages/build-info/package.jsonpackages/build-info/playwright.config.tspackages/build-info/vitest.config.tspackages/build/src/log/logger.tspackages/build/src/plugins_core/frameworks_api/skew_protection.tspackages/build/tests/unit/validate_edge_manifest/tests.jspackages/cache-utils/package.jsonpackages/config/package.jsonpackages/config/src/api/site_info.tspackages/config/src/log/messages.jspackages/edge-bundler/node/bundle.tspackages/edge-bundler/node/utils/sha256.tspackages/edge-bundler/node/validation/manifest/index.test.tspackages/edge-bundler/node/validation/manifest/schema.tspackages/edge-bundler/package.jsonpackages/edge-bundler/shared/consts.tspackages/functions-utils/package.jsonpackages/git-utils/package.jsonpackages/headers-parser/package.jsonpackages/js-client/package.jsonpackages/js-client/tsconfig.jsonpackages/nock-udp/package.jsonpackages/opentelemetry-sdk-setup/package.jsonpackages/opentelemetry-utils/package.jsonpackages/redirect-parser/package.jsonpackages/run-utils/package.jsonpackages/testing/package.jsonpackages/zip-it-and-ship-it/package.jsonpackages/zip-it-and-ship-it/src/config.tspackages/zip-it-and-ship-it/src/runtimes/node/in_source_config/index.tspackages/zip-it-and-ship-it/src/runtimes/node/parser/index.tspackages/zip-it-and-ship-it/src/runtimes/node/utils/entry_file.tspackages/zip-it-and-ship-it/src/utils/timer.tsskills-lock.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
💤 Files with no reviewable changes (22)
- packages/edge-bundler/node/bundle.ts
- packages/edge-bundler/shared/consts.ts
- packages/zip-it-and-ship-it/src/runtimes/node/utils/entry_file.ts
- packages/headers-parser/package.json
- packages/config/src/api/site_info.ts
- packages/config/src/log/messages.js
- packages/run-utils/package.json
- packages/redirect-parser/package.json
- packages/cache-utils/package.json
- packages/zip-it-and-ship-it/src/runtimes/node/in_source_config/index.ts
- packages/zip-it-and-ship-it/src/utils/timer.ts
- packages/opentelemetry-utils/package.json
- packages/edge-bundler/node/utils/sha256.ts
- packages/git-utils/package.json
- packages/build/src/plugins_core/frameworks_api/skew_protection.ts
- packages/build/src/log/logger.ts
- packages/nock-udp/package.json
- packages/functions-utils/package.json
- packages/zip-it-and-ship-it/src/config.ts
- packages/edge-bundler/package.json
- packages/zip-it-and-ship-it/package.json
- packages/opentelemetry-sdk-setup/package.json
…ve-unused-and-configure-knip
@netlify/build
@netlify/build-info
@netlify/cache-utils
@netlify/config
@netlify/edge-bundler
@netlify/functions-utils
@netlify/git-utils
@netlify/headers-parser
@netlify/api
@netlify/nock-udp
@netlify/opentelemetry-sdk-setup
@netlify/opentelemetry-utils
@netlify/redirect-parser
@netlify/run-utils
@netlify/zip-it-and-ship-it
commit: |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
hrishikesh-k
approved these changes
Aug 6, 2026
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
3 participants
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
This PR adds https://knip.dev/, a fantastic tool that reliably flags unused code and dependencies, and configures it to run in CI.
It also removes what it flagged as unused.
Note that I already did a manual pass last year: #6097.