Introduce Knip #6368
Introduce Knip #6368
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 85e58e8:
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 85e58e8. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
so the dependency doesn't show up as unused
|
@webpro here's an update after I addressed / removed most things: I think this is a false-positive. If we look at query/packages/vue-query-devtools/vite.config.cjs Lines 10 to 13 in 6c36609 So either the vite config isn't picked up, or the entry points aren't respected.
Lines 437 to 440 in 7a9d2d0 I guess JsDoc annotations aren't picked up?
query/packages/vue-query/package.json Lines 40 to 42 in 84bf775 I don't really know what that means or refers to ... For react-query, we definitely need react-dom as a peerDependency, it's not optional 😅
Those are all svelte files, wich I haven't touched, because I don't know svelte and the docs say:
I don't really dare remove those - does knip understand svelte? cc @lachlancollins maybe those can all be removed, don't know ... another thing I had to do was ignore certain at least for the |
Definitely can't remove - this is a core feature of svelte v3/v4 props (there will be a new way to do this when v5 is released, but this current way will still be supported). |
|
Getting there...
The rest we still need to do: |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #6368 +/- ##
==========================================
+ Coverage 86.35% 87.74% +1.39%
==========================================
Files 87 79 -8
Lines 2880 2832 -48
Branches 802 796 -6
==========================================
- Hits 2487 2485 -2
+ Misses 328 288 -40
+ Partials 65 59 -6 ☔ View full report in Codecov by Sentry. |
|
I think I've fixed the issue with the That last "except..." line is what's fixed today in v3.3.1 and just added to this PR. So for now I think from Knip perspective we're good :) Happy to hear any questions or feedback! |
|
@webpro And i think it's false-positive since |
It's reported as an optional peerDependency, but it's referenced. So I think it should not be optional? |
it's optional for consumers because they can also use |
|
not sure what we'd need to change this to? query/packages/react-query/package.json Lines 64 to 76 in 568ce17 |
|
Ah, now I see it's in |
|
@TkDodo Well, actually... Why do we list it as |
|
@DamianOsipiuk yes, I think you're right. We used to have This is such a nice catch - I think we can remove the peerDependency on both react-dom and react-native 🎉 |
|
here's the v4 link for reference: |
|
@webpro Seems like we are green with minimal config! Awesome 🚀 And the stuff that was found... 🤯 |
|
This is great, what a nice team effort! 😎 |
|
@webpro this is awesome, thanks for contributing! I might have missed something, can I ask why |
My pleasure!
If you look up |
Ah I see - it is a dependency of eslint-plugin-svelte so it isn't absolutely necessary, but also doesn't change anything I guess :) |
|
I think it's a pretty common best practice to try and not rely on transitive dependencies (https://knip.dev/guides/handling-issues/#unlisted-dependencies) |
5 participants
This PR introduces Knip, a tool to find unused files, dependencies and exports. It can find all sort of unused (or unlisted) things, I'll show example output below.
The current results are quite useful imho (see below for an idea).
Lots of unused exports from
__tests__files. Not sure whether test files should export anything? Those files could be ignored as well (but we'd lose their dependency usage).This monorepo is such a great exercise for Knip! If we manage to reach "inbox zero" (i.e. no reported issues) I would love to add it to Knip's integration test suite which should prevent regressions, helping both projects.
Unfortunately I did have to make one change to the codebase:
packages/vue-query-devtools/vite.config.jshas both ESM (import) and CJS (__dirname) syntax. I think Vite itself bundles its config files using esbuild before execution. This is why I renamedvite.config.jsAnother solution could be to refactor__dirnameinto ESM-compliant code.I wasn't sure if/how you'd like to a
package.jsonscript or CI workflow, so I didn't.Let me know if you have any questions or feedback, happy to discuss any concerns or questions you may have :)