Added knip#1701
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
xenova
left a comment
There was a problem hiding this comment.
Very cool! It definitely caught some old/dead code and other code smells (unused exports, etc.)
I think I need to start using knip for a currently-unreleased library I'm working on 🤫
My one minor gripe with the library is that it introduces quite a few dev dependencies, but since it's quite a well-used library across the ecosystem, I don't see this as a blocker. We also so happen to be removing many other dev dependencies in #1665, so we can consider it a fair swap haha.
PS: Big fan of @TkDodo ❤️ -- would love to see the talk if it's available online?
| "@jest/globals": "30.2.0", | ||
| "@types/jest": "^30.0.0", | ||
| "@types/node": "^24.1.0", | ||
| "@webgpu/types": "^0.1.69", | ||
| "esbuild": "^0.27.2", | ||
| "jest": "^30.2.0", | ||
| "jest-environment-node": "^30.2.0", | ||
| "jsdoc-to-markdown": "^9.1.3", |
There was a problem hiding this comment.
can you explain the jest-environment-node -> jest/globals package change?
There was a problem hiding this comment.
This was one of the suggestions from knip since we don't use it.
There was a problem hiding this comment.
Oh and about the "@jest/globals": knip found that we import @jest/globals in two places in the project but we don't have it in the dependencies. It never caused an issue because I think its still somewhere in the dependency tree, but as I said in the "onnxruntime-common", whenever we import a package directly, we should also have it in our dependencies.
| "dependencies": { | ||
| "@huggingface/jinja": "^0.5.6", | ||
| "@huggingface/tokenizers": "^0.1.3", | ||
| "onnxruntime-common": "1.24.3", |
There was a problem hiding this comment.
I remember someone mentioned this before, but since onnxruntime-node and onnxruntime-web depend on it, I don't think it's necessary to define here.
but I suppose if there is a mismatch one day between e.g., Tensor from onnxruntime-node and onnxruntime-web, it may cause some issues.
Lmk what you think!
There was a problem hiding this comment.
I think we should keep this as a direct dependency because @huggingface/transformers imports onnxruntime-common directly. And in my opinion as soon as we have a direct import, we should have it as a direct dependency and not rely on being provided by sub-dependencies.
The version mismatch concern is valid, but declaring it directly makes the resolved version explicit instead of depending on hoisting. If ORT node/web diverge further, we should probably revisit the ORT versions together rather than rely on a transitive copy.
|
@xenova, btw, the talk is online 😊: |
At React Norway I listened to a talk by @TkDodo about knip.
With this I added a knip command to npm that checks fo unused dependencies and unused code in Transformers.js. It already found a couple of lines. @xenova, do you think it makes sense to keep it in the project?
Summary
packages/transformers.knipCI job in the unit test workflow using Node.js 24.10.0.