-
-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add knip plugin #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
commit: |
} else if (stat.isDirectory()) { | ||
// If it's a directory, use it as the root for local file system | ||
// Set pack to 'none' to use LocalFileSystem | ||
pack = 'none'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of this?
the pack type mirrors whats in publint so im surprised this works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the purpose of setting pack = 'none'
when a directory is detected is to use LocalFileSystem
instead of TarballFileSystem
.
the current behavior:
No path provided → pack = 'auto' → packs directory → TarballFileSystem → publint ✅ works
Directory path provided → pack = 'none' → LocalFileSystem → publint ❌ skipped
The inconsistency is that directory paths bypass the packing process while everything else uses tarballs. This is why knip works with directories but publint/attw don't.
"Always pack, separate knip command" would mean:
- Remove the pack = 'none' override for directories
- Always pack directories (like the default behavior)
- Add a separate command/flag for knip analysis that works with local filesystem
so let me know if we should refactor it.
|
||
async function which(cmd: string): Promise<string | null> { | ||
return new Promise((resolve) => { | ||
const proc = spawn('which', [cmd]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe which
will run on Windows. Is that a concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello @michaelfaith, nice catch, but i don't think it is a concern right now, since we are in early development 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also we stumbled upon a big rabbit hole with this one that's why it has become stale.
Add knip as peer dependency plugin with feature flags
Implements #41 by integrating knip as an optional peer dependency that runs via
npx
.Changes
analyze
command--features
flag controls which analysis tools run:--features unused
- runs only knip--features types,publish
- runs attw + publint, skips knipUsage
Closes #41