Update knip + config #2647
Update knip + config #2647
Conversation
webpro
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes primarily involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Config as KnipConfig
participant Package as package.json
Config->>Config: Update ignore paths
Config->>Config: Restructure workspaces
Package->>Package: Remove workspaces field
Package->>Package: Update knip version
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@webpro is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
| @@ -1,6 +1,5 @@ | |||
| { | |||
| "name": "unkey", | |||
| "workspaces": ["apps/*", "packages/*"], | |||
There was a problem hiding this comment.
This confused Knip. Can it be removed?
|
|
||
| const config: KnipConfig = { | ||
| ignoreWorkspaces: ["packages/nuxt"], | ||
| ignoreDependencies: ["cz-conventional-changelog"], |
There was a problem hiding this comment.
Add commitizen explicitly to activate the plugin and this wouldn't be reported.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
knip.ts (1)
1-34: Consider enhancing Knip plugin ecosystemThe configuration relies on manual entry point specification for trigger.dev and trpc due to missing Knip plugins. Consider:
- Contributing plugins for trigger.dev and trpc to the Knip ecosystem
- Documenting the manual entry point patterns in a central location for maintainability
- Setting up automated validation of entry point coverage
This would improve maintainability and reduce the risk of missing important entry points.
package.json (1)
Line range hint
1-58: Critical: Restore the "workspaces" fieldThe removal of the "workspaces" field is concerning as it's essential for pnpm to manage dependencies in a monorepo setup. This could break the workspace management and inter-package dependencies.
Apply this diff to restore the workspaces configuration:
{ "name": "unkey", "private": true, + "workspaces": [ + "apps/*", + "packages/*" + ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
knip.ts(1 hunks)package.json(1 hunks)
🔇 Additional comments (8)
knip.ts (7)
1-3: LGTM: Clean type import and configuration setup
11-13: LGTM: Root workspace configuration
The root workspace entry point is clearly defined.
29-31: LGTM: K6 tool configuration
The k6 tool entry point is clearly defined and specific.
7-9: Verify other non-JS/TS applications
The comment indicates that apps/agent is ignored as it's a Golang app. Let's verify if there are other non-JS/TS applications that should be ignored.
#!/bin/bash
# Description: Find directories that might contain non-JS/TS applications
# Look for Go files
echo "Directories with Go files:"
fd -e go . apps internal packages tools -x dirname {} \; | sort -u
# Look for other common non-JS/TS project indicators
echo -e "\nDirectories with other language indicators:"
fd -e rs -e py -e rb -e java -e cpp . apps internal packages tools -x dirname {} \; | sort -u21-28: Verify trpc entry points coverage
The broad glob patterns for the dashboard app might include unnecessary files or miss important ones. Let's verify the coverage.
#!/bin/bash
# Description: Verify TRPC-related files coverage
# Find all TypeScript files in app directory
echo "TypeScript files in dashboard app directory:"
fd -e ts -e tsx . apps/dashboard/app
# Find files with TRPC imports or usage
echo -e "\nFiles with TRPC imports or usage:"
rg -l "@trpc|createTRPCNext|createTRPCProxyClient" apps/dashboard
# Verify lib/trpc paths
echo -e "\nVerify lib/trpc structure:"
ls -la apps/dashboard/lib/trpc/14-20: Verify trigger.dev entry points coverage
The comment indicates that Knip lacks a trigger.dev plugin, leading to manual entry point specification. Let's verify the coverage of trigger-related files.
#!/bin/bash
# Description: Verify trigger.dev related files coverage
echo "Trigger-related files in apps/billing:"
rg -t ts -l "trigger" apps/billing
echo -e "\nCompare with specified patterns:"
echo "- trigger.config.ts"
echo "- src/trigger/*.ts"4-6: Verify other workspaces without package.json
The comment indicates that artillery is ignored due to missing package.json. Let's verify there aren't other similar cases that should be included.
✅ Verification successful
Artillery is indeed the only workspace without package.json
The verification confirms that among the tools directories, only tools/artillery/ lacks a package.json file, while all other directories (k6, local, and migrate) have one. This validates the current ignore configuration as correct and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find directories that might be workspaces but lack package.json files
# Search for potential workspace directories (excluding node_modules and common build directories)
echo "Directories without package.json that might need to be ignored:"
fd --type d --min-depth 1 --max-depth 2 \
--exclude 'node_modules' --exclude 'dist' --exclude '.next' \
. apps tools internal packages \
--exec sh -c '[ ! -f "{}/package.json" ] && echo "{}"'
Length of output: 597
Script:
#!/bin/bash
# Let's try a different approach to find directories without package.json
# First, let's list all directories in tools/ to understand the structure
echo "Directories in tools/:"
fd --type d --max-depth 1 . tools/
echo -e "\nChecking each directory in tools/ for package.json:"
for dir in $(fd --type d --max-depth 1 . tools/); do
if [ -f "$dir/package.json" ]; then
echo "✓ $dir (has package.json)"
else
echo "✗ $dir (no package.json)"
fi
done
Length of output: 978
package.json (1)
30-30: Verify compatibility with updated knip version
The knip package has been updated from ^5.11.0 to ^5.36.2. While this is a minor version bump, it's a significant jump that could introduce breaking changes.
Let's verify the changelog and potential breaking changes:
✅ Verification successful
Knip update appears safe to proceed
The update from 5.11.0 to 5.36.2 appears safe as:
- The project uses a simple knip configuration with basic flags (
--include files,dependencies) - No custom knip configuration files are present
- The usage is limited to a single npm script for dependency analysis
- The version bump follows semver (^5.x.x), indicating no breaking changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes in knip updates
# Test 1: Check if knip is used in any npm scripts across workspaces
echo "Checking knip usage in package.json files..."
fd package.json --exec rg -l "knip"
# Test 2: Check for any custom knip configurations that might need updates
echo "Checking for knip config files..."
fd -e ts -e json -e yaml -e yml --exec rg -l "knip.*config"
# Test 3: Verify if the new version is compatible with the current Node.js version
echo "Checking knip peer dependencies..."
rg '"engines"' package.json -A 5
Length of output: 402
Script:
#!/bin/bash
# Let's check the actual usage and configuration of knip
# Check package.json content for knip scripts and config
echo "Checking package.json content..."
cat package.json
# Check for .knip.json or knip.json
echo -e "\nChecking for knip configuration files..."
fd knip.json
# Check for any knip configuration in package.json
echo -e "\nChecking for knip configuration patterns in package.json..."
rg '"knip":' package.json -A 10
Length of output: 2650
| entry: ["lib/trpc/index.ts", "trpc.config.ts"], | ||
| }, | ||
| "apps/api": { | ||
| entry: ["**/*.test.ts", "src/pkg/testutil/*.ts", "src/worker.ts", "./vitest.*.ts"], |
There was a problem hiding this comment.
Good example of entry files Knip figures out itself, none of those are needed :)
|
thanks @webpro |
|
There you go |
What does this PR do?
Ref: https://twitter.com/chronark_/status/1853744727037780324
Knip has a lot of defaults and heuristics to fill out the entry points for you.
I went through all
apps/*folders, removed all config and added back a little (for the same results). I did not go throughinternal/*,packages/*andtools/*.Lots of results, and a quick scan tell me they're mostly not false positives.
Type of change
How should this be tested?
Since the monorepo is pretty large, it might be worthwhile to pick one workspace at a time, eg:
This will take into account dependent and depending workspaces as well. So e.g. if a workspace references a dependency that's only listed in the root
package.jsonthen it won't be reported (unless using--strictmode).There's a lot more to it, the docs should cover plenty. Yes, it takes a bit of effort in a monorepo like this, but long-term it should def be worth it. Key takeaway: results are usually suspicious: deal with the reported thing, add as entry, or report bug in Knip :)
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Chores
knippackage to the latest version for enhanced functionality.