🐛 Abstracted lazy imports are not detected

[EmNudge]

Prerequisites

• [X] I'm using the latest version
• [X] I've read the relevant documentation
• [X] I've searched for existing issues
• [X] I've checked the list of known issues
• [X] I've read the issue reproduction guide

Reproduction url

https://stackblitz.com/edit/github-yu5k5m-vxck8v?file=index.ts

Reproduction access

• [X] I've made sure the reproduction is publicly accessible

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

const Component2 = lazy(() => import('./Bar').then(({ Bar }) => ({ default: Bar })));

However, the following isn't (understandably)

const Component = lazyImport(() => import('./Foo'), 'Foo');

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 for Baz, 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 option

This 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

importList: {
  // path it's imported from
  './foo/bar/baz.ts':  {
    // path it's importing
    './foo.tsx': [
      // named imports
      'Foo', 'Bar'
    ]
  }
}

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 of Foo as an import.

[webpro]

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.

[webpro]

Going to close this one, it's just too custom and workarounds are available (ignore, custom reporter).