Detect unused JSX component props

[SanderRonde]

Implement detection of unused JSX component props. Example:

interface ComponentProps {
    foo: number; 
    bar: string; // <-- This is now unused
}

const MyComponent: React.FC<ComponentProps> = (props) => {
     ...
}

const App = () => <MyComponent foo={1} />

Implementation:

• Collect all JSX components (currently only has a React component visitor). Do this by checking for a number of patterns (see packages/knip/src/typescript/visitors/jsx-component/reactComponent.ts for more details). In this process collect the name of the Props type
• In main function iterate over all components
• For each component find the associated Props node again
• Iterate over props' members
• For each member, find all references
• For each reference, check if this reference is a JSXAttribute (i.e. <MyComponent foo />). If there are none, this prop member is marked as unused
• Add this new error type as possible output

Limitations:

• Does not detect non-top-level props. For example in the case interface Foo { unused: boolean }; interface ComponentProps extends Foo { } unused is not detected
• Does not detect non-JSX methods of passing props to components. For example React.createElement(..., { foo: 'bar' }) or <Component {...{foo: 'bar'}} />. This is quite simply because TS does not detect it as a reference to the props field.

I have to admit that I haven't used Knip at all before yesterday and I'm not really all that knowledgeable of how error reporting/fixing works in Knip so I think there may still be some code lacking there (see TODOs in the code too), so feel free to give me some pointers.

[webpro]

Thanks for the PR, @SanderRonde. I really appreciate it.

This feature will be appreciated by many and adds a lot of value, so let's see how we can deliver.

1. As mentioned briefly in https://knip.dev/reference/faq#why-doesnt-knip-have, I'm definitely open to the idea of this pull request.

2. About a main design choice: in the main sequence/function (src/index.ts), the aim is to generate the graph and collectUnusedExports from it. This graph should be serializable (for a few reasons, e.g. caching, auto-fix, and maybe future exposure of this graph for other libs), and should not contain functions or AST nodes (probably running knip --cache twice currently fails). Good thing is that we didn't expose graph yet so we can still tinker with ideas as presented in this PR.

3. Another major thing we'd be introducing here is including non-exported values/types in the analysis. Currently only members of exported enums and classes are taken into account.

4. The addition of React component prop types is a bit specific. React is popular, but Knip is more of a generic tool. If we would merge this, even though we could opt-in such features behind a flag, we might end up with more of such relatively heavy AST visitors. This is also a maintenance issue I don't really fancy taking up atm. However, if we extend the idea to all types and interfaces and have a good generic approach things might be even more powerful and simpler at the same time.

So, overall, I think what we need to tackle first is two things:

1. If and how to introduce the concept of values and types that are not exported? Both in terms of implementation as well as representation in or out of graph.
2. If and how to introduce new visitors that allows to implement this specific or a more generic use case?

If we get those right, I'd expect the implementation in the main sequence to become easier:

• treat type and interface members the same way as classMembers
• we can use logic similar to collectUnusedExports:
  • depends on what we'll have in/out of file.exports?.exported
  • extend exportedItem.type === 'class' with type === 'type and type === 'interface)
  • have principal.findRefsForMembers (i.e. languageService.findReferences) do the hard work for us

Sorry, that's a lot to digest perhaps. I'm going to let it simmer for a bit. Happy to hear your thoughts on any or all of this and discuss further. Thanks again for pushing this.

[SanderRonde]

Some high-level thoughts on the to-tackle things:

1. If and how to introduce non-exported types:
   • On whether we should: I don't think it's strictly necessary to use non-exported values. What we could do is to look for all exported JSX/React components, go over their props (that may or may not be exported) and add any props-information to the visitor result. That way the main components are always exported and the dependencies (in the form of props) don't need to be and are stored on the component. Some downsides to this are:
     • This does of course exclude non-exported components from checking, which is a design choice
     • The pattern of const Component = (props) => { ... }; export default Component; is (I think) very common in React-based projects. We'd need to, when we encounter an export default resolve the original component again and mark that as an exported component. But this should be doable.
   • If we make it use exported-only fields it should indeed be as simple as changing the shape of interfaces/types. This was actually my first implementation and I still have it stashed somewhere. Might be nice to use that as a starting point and to rewrite this to more-so use the existing graph.
2. After thinking about this some more, I think we might also be able to include cases like export function someDateFormatter(someDate: Date, options: DateOption) { } in this check too. So we can turn the definition into checking whether any exported function has an interface with unused members (which is a lot more generic). Key detail is that those members should be able to be used within that function itself of course without it being marked as "external usage". I do wonder if that's as much of a feature that people would request though, and whether it's something that might be "annoying" when somebody just wants to check React components.

Happy to hear what you think

[webpro]

Yes, we could actually start by taking only exported interfaces and types into account.

The downsides of this approach I can think of:

• people might complain the feature's "half-baked" and/or
• start exporting types and interfaces for this to work and then ignore the same unused exports

Since it'll be an opt-in feature (like classMembers) I don't see it as a huge issue, we could also keep it under an "experimental" flag until it's more complete.

On whether we should: I don't think it's strictly necessary to use non-exported values. What we could do is to look for all exported JSX/React components, go over their props (that may or may not be exported) and add any props-information to the visitor result.

• The idea of the current setup and AST visitor functions is that we can traverse each file's tree only once efficiently without hopping all over the place.
• We can make our lives a lot easier if we consider type and interface members and let LS.findReferences do the heavy lifting, this should then cover lots of grounds including React component props - or do you think we'd be missing something?

[SanderRonde]

Yes, we could actually start by taking only exported interfaces and types into account.

Hmm in that case I do indeed think that indeed it's not as useful. I think the pattern of exporting a React component and not its types is very common so for that use case it would indeed not be that complete. Could maybe consider requiring the components/functions that use those those interfaces/types to be exported instead, although I do admit that then you're no longer really "only checking exported values". Maybe it's worth considering dropping the exported-only requirement then. It would indeed be best for this to work perfectly on the first go. And while it's fine to improve on the feature while it's opt-in, I think it's good for the "final goal" to be as complete as possible.

The idea of the current setup and AST visitor functions is that we can traverse each file's tree only once efficiently without hopping all over the place.

Ah yeah I just forgot that it's indeed a valid case for an interface/type to be non-local to the file, meaning you would have to crawl another file. However I guess in that case it is already in the exports field in the graph so we could maybe read from there?

We can make our lives a lot easier if we consider type and interface members and let LS.findReferences do the heavy lifting, this should then cover lots of grounds including React component props - or do you think we'd be missing something?

Yeah that should work. Indeed only caveat is that you'll need to filter out usages within the body of usages of that interface. For example a component reading from props.foo in its function body should not count as an "external" usage and should not prevent it from being marked as unused. That then does complicate things in that we will have to find the instances in which an interface is used in a component/function and go through that.

Or another (very iffy) approach might be to iterate through findReferences and to determine whether it's an external usage based on the type of node. For example a JSXAttribute would count as external usage, so would a field being used as an object key someFunction({ foo: 'bar' }). This would break for recursive functions and I'm not sure if this would fully cover all cases. But it could save a loooot of time and processing of all references.

[webpro]

A few more things to consider perhaps:

• Your use case is React component props, I'm thinking interface and type members in general. We'll get to both :)
• Finding unused props will likely end up not being included by default, much like classMembers, because of the dependency on LS.findReferences. This is because references to enum members (and also namespace imports) is mostly simple "property access". While class members - and I suspect the props area you're looking into as well - is a lot more complicated and expensive, so is delegated to the expensive but excellent LS.findReferences (wrote more here: https://knip.dev/blog/slim-down-to-speed-up#the-story-of-findreferences).
• Not having all the various ways of referencing type and interface members in my mind yet, let's expect the worse (i.e. most will require LS.findReferences).

Yeah that should work. Indeed only caveat is that you'll need to filter out usages within the body of usages of that interface. For example a component reading from props.foo in its function body should not count as an "external" usage and should not prevent it from being marked as unused. That then does complicate things in that we will have to find the instances in which an interface is used in a component/function and go through that.

Not sure I fully understand. Could you give an example, perhaps with code, what needs filtering out from regular logic of (not) being referenced?

[SanderRonde]

Your use case is React component props, I'm thinking interface and type members in general. We'll get to both :)

Yes, I do think that it would be really nice for both of these to work with the same implementation. But my fear is that if we're going with the approach of only checking exported interfaces/types, it might not work that well for React component props. But let's see :)

Finding unused props will likely end up not being included by default

Ah yes that makes sense

Not having all the various ways of referencing type and interface members in my mind yet, let's expect the worse

Agreed!

Not sure I fully understand. Could you give an example, perhaps with code, what needs filtering out from regular logic of (not) being referenced?

Of course! Here's a generic case in which we'd need some filtering

// date.ts
export interface DateFormatOptions {
    weekday?: boolean; // <-- Unused
    month?: boolean;
}

export function someDateFormatter(date: Date, options: DateFormatOptions) {
    const text = [];
    if (options.weekday) { // <-- This counts as a reference according to LS.findReferences and it's making this not-unused
        text.push(date.toWeekDay());
    }
    if (options.month) {
        text.push(date.toMonth());
    }
    return text.join(' ');
}

// app.ts
const date = someDateFormatter(new Date(), { month: true });

[webpro]

Yes, I do think that it would be really nice for both of these to work with the same implementation. But my fear is that if we're going with the approach of only checking exported interfaces/types, it might not work that well for React component props. But let's see :)

Yeah we'll eventually need that unexported part too for sure, but gotta start somewhere :)

Of course! Here's a generic case in which we'd need some filtering

Right. Not sure I agree this should be filtered out. Did you consider that data can come from anywhere/outside the codebase as well:

const data = await fetch() as OurInterface;
return <SomeExternalComponent {...data} />

Thinking we should keep it simple and see whether props are referenced in/from our own code. Also see https://knip.dev/guides/namespace-imports for how we could go about cases like this, but as mentioned before, not having all the cases clear yet.

[SanderRonde]

Right. Not sure I agree this should be filtered out

Hmm if they wouldn't be filtered it would be quite rare for an options-style interface to ever have any of its members marked as unused right? In the example I sent, even though the weekday member is never referenced outside of its "implementation" it would not be flagged as unused. The same goes for React components, where there will generally be an implementation for an unused prop/member, just not any usages of that prop. That's not to say there aren't plenty of cases in which this would help of course, but it does greatly reduce the use case for React components. Of course this usefulness for React components could always be added later.

Did you consider that data can come from anywhere/outside the codebase as well:

Yeah indeed that does complicate things. Even if that data doesn't come from outside the codebase, an object spread being passed to a JSX component isn't seen as a reference by TS. For example in const someProp = {foo: 'bar'}; <SomeComponent {...someProps} /> TS does not find this usage when using "find all references" on SomeComponentProps.foo.

Thinking we should keep it simple and see whether props are referenced in/from our own code

Yeah agreed. Although in this case a downside of less-thorough detection in finding references means false positives (members flagged as unused even though maybe a fetch call does indirectly use them) rather than false negatives right?

[webpro]

A type/interface and the object passed as component props are two different things: in your example, DateFormatOptions.weekday is used and referenced, but options.weekday is not. It's important to make that distinction and I've been mostly talking about the types part, not the object props. The use case you're after is a different beast which I haven't thought through at all yet, that's more like finding "passed argument prop usage". That's two different issue types.

[webpro]

Which isn't to say we should or could not implement the use case, we just need to think it through a bit more: how to opt-in, where to hook this special case into, without too much added complexity nor sacrificing performance during the default flow. Also wondering whether this information should be in the graph (it's purpose is to connect nodes in the project through imports and exports), and where to store it then. Knip feels like the right tool for the job, but also it feels not ready for this yet. Absolutely open to ideas and suggestions and further discussion, it has been very valuable to me already.