webpro-nl / knip

✂️ Find unused files, dependencies and exports in your JavaScript and TypeScript projects. Knip it before you ship it!
https://knip.dev
ISC License
7.03k stars 174 forks source link

🐛 TS4020 and TS4023 from removing reported unused export if exported type is referenced implicitly in another package #801

Closed evelant closed 1 month ago

evelant commented 1 month ago

Prerequisites

Reproduction url

https://stackblitz.com/edit/github-a4rrlb?file=README.md

Reproduction access

Description of the issue

Knip reports false unused types when those types are used transitively in another package and are required to be exported otherwise ts fails to build with TS4020 or TS4023 such as error TS4023: Exported variable 'bar' has or is using name 'Error1' from external module "/home/projects/github-a4rrlb/packages/shared/dist/file2" but cannot be named.

See the readme of the linked reproduction for exactly how this can happen

webpro commented 1 month ago

This reproduction is great, thanks a lot for setting that up.

A workaround I can give you now is to add this to knip.json:

{
  "ignoreExportsUsedInFile": {
    "enum": true,
    "interface": true,
    "type": true
  }
}

Also see https://knip.dev/reference/configuration#ignoreexportsusedinfile.

However, I'm going to give it a closer look soon to see if I this can be improved, because at first sight I do think it's unexpected behavior and a bug in Knip (that's why I call it a workaround).

evelant commented 1 month ago

I'd rather avoid that workaround because I'm specifically in a situation where someone made lots of things exported when they're only used internally. I'd like to clean those up, but this issue makes it a bit difficult. I suppose what I'll probably do is just run ts in watch mode and add a /* knipignore / for any exports that cause this problem. I'm not so sure this is a bug in knip, IMO it's a bug in typescript but I don't think the typescript team sees it that way so it probably won't change and it would be great if it were possible for knip to detect these cases.

webpro commented 1 month ago

Gotcha. I do think it's an issue with Knip. You could try this:

npm i -D https://pkg.pr.new/knip@e39014e

If that works as expected I can include it in a next release.

evelant commented 1 month ago

Hmm that build seems better, but I'm still having some false positives. It seems likely the false positives are a config issue on my end however.

webpro commented 1 month ago

OK, sounds good. Feel free to update the repro now that you've set that up already, happy to look into cases.

evelant commented 1 month ago

@webpro a question unrelated to this issue -- I'm getting false positives because I have a react-native project in my monorepo and react-native allows resolving files via platform specific extensions. For example I could have MyFile.ts, MyFile.native.ts, MyFile.web.ts, MyFile.ios.ts or MyFile.android.ts. Knip is marking all .web.ts and .native.ts files as unused because they are imported without their extension by design in react-native like import { foo } from "./MyFile". I'm not sure how to configure knip to handle this. Do you have any advice?

webpro commented 1 month ago

Re. RN, bad news unfortunately: https://github.com/webpro-nl/knip/issues/126

webpro commented 1 month ago

Hmm that build seems better, but I'm still having some false positives. It seems likely the false positives are a config issue on my end however.

Think there might've been an oversight on my end. For now we need to enable that ignoreExportsUsedInFile, can you try it with that build and a minimal setting that's not bothering the results too much:

{
  "ignoreExportsUsedInFile": {
    "member": true,  // or "function": true
  }
}
webpro commented 1 month ago

By the way, a workaround for the RN situation is to just add files like **/*.{android,ios,web}.ts as entry files. The only downside is that you'd miss out on unused files in this category of files.

evelant commented 1 month ago

Ah I see, yeah, react-native can be a painful platform to work on. It deviates from standards is so many places with its own bundler and js runtime. If I'm understanding the docs correctly then adding any .native.ts files as entrypoints would prevent them from being reported (even if they really are unused) and ensure that anything they import gets added to the graph. Does that sound right?

I don't have a lot of files with platform specific extensions so it's not much of a problem to maintain them manually, I just want to avoid knip giving false positives in other parts of the app because it didn't pick up a file with a platfrom extension.

webpro commented 1 month ago

If I'm understanding the docs correctly then adding any .native.ts files as entrypoints would prevent them from being reported (even if they really are unused) and ensure that anything they import gets added to the graph. Does that sound right?

Exactly. Except if includeEntryExports is set (can be set per workspace).

webpro commented 1 month ago

My best shot at it so far:

npm i -D https://pkg.pr.new/knip@e4bada4

Thanks for raising this, will likely be released soon.

webpro commented 1 month ago

Closing this by releasing v5.32.0

aaayane commented 1 month ago

Description of the issue

Knip reports false unused types when those types are used transitively in another package and are required to be exported otherwise ts fails to build with TS4020 or TS4023 such as error TS4023: Exported variable 'bar' has or is using name 'Error1' from external module "/home/projects/github-a4rrlb/packages/shared/dist/file2" but cannot be named.

See the readme of the linked reproduction for exactly how this can happen

Hi, thanks for your work on this project. I think I have a similar problem which doesn't seem to be solved in the new version.(knip 5.33.2). When exporting a function, the reference type will still prompt that it is not used

Reproduction url

https://stackblitz.com/edit/github-a4rrlb-3x1iep?file=README.md

Description of the issue

When exporting a function, the reference type will still prompt that it is not used TS4023: Exported variable 'sharedFunction' has or is using name 'Error1' from external module "/home/projects/github-a4rrlb/packages/shared/src/file2" but cannot be named.

See the readme of the linked reproduction for exactly how this can happen

webpro commented 1 month ago

Thanks @aaayane. Sounds like a valid bug report indeed. Any chance you could open a new issue for this? You can just copy-paste. Thanks!

aaayane commented 1 month ago

Thanks @aaayane. Sounds like a valid bug report indeed. Any chance you could open a new issue for this? You can just copy-paste. Thanks!

Thanks for the reply, I have created a new issue. Here is the link https://github.com/webpro-nl/knip/issues/808