Open EmNudge opened 2 weeks ago
Custom resolvers, AST visitors, etc are on the back of my mind. At this point I just don't fancy opening up more API surface area.
However, for the time being, what you describe as the import-list
option can be done already using a preprocessor.
The preprocess function will receive the reporter data like this:
{
"issues": {
"exports": {
"Foo.tsx": {
"Foo": {
"type": "exports",
"filePath": "Foo.tsx",
"workspace": ".",
"symbol": "Foo",
"symbolType": "unknown",
"pos": 13,
"line": 1,
"col": 14,
"severity": "error"
}
}
}
}
}
So Foo
is incorrectly an issue, which you can filter out using the data you have found in your script. The preprocess function should return the modified data and it will be passed to the reporter.
Prerequisites
Reproduction url
https://stackblitz.com/edit/github-yu5k5m-vxck8v?file=index.ts
Reproduction access
Description of the issue
Reopening issue https://github.com/webpro-nl/knip/issues/616 as per the closing comment.
Problem Restatement
React lazy's default implementation creates for some undesirable behavior, so many projects will wrap it using a factory pattern. Knip's lack of support for this (emitting false positives) makes it a present non-starter for many large react projects.
The following code is properly caught by Knip
However, the following isn't (understandably)
Responses to previous feedback
The component is typed as
any
The previous stackblitz didn't have
@types/react
installed. This has been fixed.Try
--include-libs
No effect
Knip doesn't work if "find all references" doesn't
This would affect almost all uses of
react.lazy()
which is meant to work with default exports. If you try finding all references forBaz
, it won't resolve. Luckily, Knip still catches this one.I don't think it's fair to make this the bar.
How Knip could help here
As per the previous discussion, Knip not catching this is fair. We're abstracting the behavior across multiple functions, so it's not clear that
'Foo'
is an export from./Foo.tsx
. However, it is statically analyzable, just non-standard.Here are some ideas for solutions that I think would work for most projects like this
Supporting an
import-list
optionThis one is a bit messy but it technically solves the problem by allowing the consumer to do the work of resolving the imports.
I can run a script which finds all the imports and which file they're imported. It might look something like
I would pass this option to Knip right before running and Knip would then keep this information in mind when resolving imports. It would treat it the same as if it say
import { Foo } from './Foo'
Supporting custom resolvers
This one might be more perf-intensive, but it's opt-in. For any file touched, run a script which defines the imports. Same as above, but doesn't force us to walk the tree ourselves.
Include any unresolved lazy imports
Less fun since it'll probably miss a bunch of cases, but it would help get rid of the absolutely massive amount of false-positives reported by Knip currently.
If knip sees
() => import('./Foo')
then include all named exports ofFoo
as an import.