-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
chore: configure knip #13016
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
chore: configure knip #13016
Conversation
|
@@ -14,6 +14,7 @@ | |||
"strict": true, | |||
"allowJs": true, | |||
"checkJs": true, | |||
"skipLibCheck": true, |
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.
this is probably the bigger change than moving the knip config. but i guess its fine because svelte itself does not consume libs in a way we would gain something from these checks?
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.
yeah, I'm seeing some annoying errors due to some type changes in a dependency that are totally irrelevant to us and to Svelte consumers. I don't see much value in checking libs
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.
We can't really do this, we won't be able to catch errors in our own d.ts files from now on. The alternative is to turn all our d.ts files into ts files which will be type checked
As @dominikg pointed out recently, it's a bit weird to include
knip
config in thepackage.json
that gets included in thesvelte
package.This PR moves it into a separate
knip.json
file, beefs up the configuration to cut down on some of the noise, and acts on some of its recommendations (yes, this includes installing certain workspace-wide dependencies insidepackage/svelte
; an alternative would be to move theknip
config into the root).With this change, the only warnings that are left are ones that appear to be legitimate:
Ultimately I'd like to run
knip
in CI, but that can happen separately.