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.

[SanderRonde]

A type/interface and the object passed as component props are two different things

Yeah I think we're indeed both looking for 2 main goals here. My primary goal is/was JSX component checking (or a more generalized version if possible), yours is primarily interface checking. I think we're indeed both hoping for the "final" state to be one where Knip does both of them.

Knip feels like the right tool for the job, but also it feels not ready for this yet

Yeah I think I'm going to have to agree here. My main reason for questioning whether the approach of doing interface checking first and then growing that into component-checking was the right one was that indeed it sounds like component-checking might not "fit" very well into the pattern of interface-checking and into Knip's current design. As you already listed:

• This would need to be collected differently during the building of the graph
• Performance should be taken into account
• It turns the graph from something relatively "neutral" into something that now has quite implementaton-specific elements in it
• It would also need to collect non-exported nodes
• It should maybe also need to collect references to other nodes (example: MyComponent also now needs to find MyComponentProps)

These all sound like relatively big "issues" and not like something that would fit very well on top of something like interface member checking (which fits the current philosophy very well). For that reason I'd say it's maybe better to, if this is indeed implemented in Knip, implement it as a separate visitor and in an opt-in like state. Similar to the way done in this PR.

I'm of course still willing to help you out in getting there, but it feels like the interface-member checking might be better implemented by you given that it's relatively straight-forward and I'm sure you know much more about the specifics of implementing Knip features than I do :). This PR is also relatively far from an implementation of that. I am of course always willing to further discuss and work on the JSX component props checking and to think about how to (and whether to) fit it into Knip.

[webpro]

💯

• type and interface-member checking is def something that'd be easy for me to add
• feel free to tinker/discuss more re. JSX component props checking, as you were already starting this PR for that
• staying in this area, going to mention this only because it might help us define a strategy for collecting/handling similar types of concerns, this has been asked for a few times: check unused props in objects returned from (exported?) functions, eg:
  • Find unsed methods of objects #274
  • detect unused fields returned by setup function #750
  • Add support for Vue composables unused properties detection #756

[SanderRonde]

type and interface-member checking is def something that'd be easy for me to add

Awesome!

feel free to tinker/discuss more re. JSX component props checking, as you were already starting this PR for that

Sounds good. I'm of course very curious about your thoughts so I'll just throw my general idea at you and let's see what you think :)

• I think we agree that this does not really fit super well into the general idea of Knip, mainly things like this also targeting non-exports, something quite specific (JSX vs types) etc. For this reason I think best to separate the code from the regular Knip flow/setup where possible to prevent complicating the regular flow.
  • Because of this: I'd say best to add a separate visitor for this and a separate entry in the graph
  • Similarly best for this to be opt-in
• I think it can definitely be generalized to just "unused argument members of exported functions". From that point on maybe it's helpful to add some ReactComponent filters or something on top, but let's leave that for now.
  • This general idea would also support (Find unsed methods of objects #274) if we'd support returned interfaces/types too, which is not that complex I think
  • Regarding detect unused fields returned by setup function #750 and Add support for Vue composables unused properties detection #756 I think the fact that there's no explicit return type complicates this a lot. While TS is technically capable of doing this, it would involve a lot of very heavy overhead. Namely:
    • Find function node
    • Iterate through body to find all top-level return statements
    • Get the returned value (could be that you return { foo: () => { ... } } or that you const foo = { bar: () => { ... } }; return foo)
    • Find the original declaration of each member (so the foo identifier)
    • Run findAllReferences on that
    • Sidenote: I'm also very afraid that this will flag a loooot of false positives
• Implementation wise I think the flow I initially proposed (and implemented) in this PR would work pretty well. With some small changes:
  • Don't collect just JSX components, collect all exported functions and references to their interface/type args (if they have any)
  • Instead of checking if each reference is JSXAttribute instead check whether the reference falls inside the function body

[webpro]

"unused argument members of exported functions"

💯

Is that enough for "unused JSX component props" though?

Also, totally on board with leaving out any complications re. exported object members as you mention.

Implementation wise

A few things to consider here. The graph (DependencyGraph) is intentionally serializable:

• Caching (e.g. can't have functions nor AST/nodes)
• Graph should be consumable without dependency on TypeScript
  • Other libs may consume it
  • That's why IssueFixer works w/o AST nodes (but w/ character positions instead)

Previously I've thought of having a separate graph/storage of sorts before for this type of functionality, but now I'm on board with extending the current graph, provided the extra information stays optional. So we would need to extend DependencyGraph and serialize the information we need for the reporting.

• Implementation-wise we could go "all out" during AST traversal (some things might get expensive indeed), as long as we agree on a good contract (graph definition/structure) first and serialize to this properly. Then we can optimize both ends separately (input/visitor vs output/reporting).
• Re. those type definitions, I'm on the fence at this point whether FileNode should have a new member to add the new data for exported functions (like so), or we should attach it to Export directly. The latter will conflict if we'd want to support components that aren't exported so I'm leaning towards the former (also more focused/cleaner to start out with?).
• Re. symbol?: ts.Symbol types in there: they're optional and deleted in getImportsAndExports (i.e. must not end up in graph for serializability).
• Ideally, eventually, we can autofix reported issues, but this is not required nor does it need to be done in the same PR. Let me know if you'd like me to expand on this (the gist: the fixer eats exports/members inside-out based on source file character pos, in reverse).

[SanderRonde]

Sorry for the rather late response

Is that enough for "unused JSX component props" though?

I think it should be yeah. We could however choose to more specifically cater to JSX components and drop the generic-ness. I think approaching this generically might have too many false-positives to be worth it:

• API data will almost always be flagged
• Let's say 2 functions use the same interface as an arg, then a reference to an interface member inside of one function implementation will count as a valid reference and thereby making it unused. For example:

interface SomeInterface {
    foo: number;
}

function fn1(options: SomeInterface) {
    if (options.foo) { return 1 } // <- We can't filter this out unless we also collect all non-exported functions that reference an interface, which tbh is probably not worth the effort.
}

export function fn2(options: SomeInterface) {
    if (options.foo) { return 1 } // <- We can "filter out" this usage because it's in a function implementation
}

A few things to consider here. The graph is intentionally serializable:

Alright that's good to know. That would essentially mean that we'd need to make a few changes. Right now the implementation in this PR is the following:

• Collect all JSX components (currently only has a React component visitor). 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
  • This is a TS dependency and should not be done here. Should instead be done during the process of collecting.
• Iterate over props' members
  • Also a TS dependency, could also happen during collecting
• For each member, find all references
  • This is fine I think
• For each reference, check if this reference is a JSXAttribute (i.e. ). If there are none, this prop member is -marked as unused
  • While this is somewhat of a TS dependency, it's not a TS dependency on the data that is consumed so I think this should be fine right?
• Add this new error type as possible output

So I think the new flow should be:

• Collect all exported functions with an interface/type as an argument.
  • Still during collection, find the original definition of that interface
  • Find all members of that interface, record the identifier/position/etc
  • Store this data in the dependency graph (not necessarily on exports but something else)
• In main function iterate over all functions
  • Join together any references to the same interface such that we can report once per interface
  • Iterate over members
  • For each member, find all references
  • For each reference, check if this reference is a JSXAttribute (i.e. ). If there are none, this prop member is -marked as unused
  • Report errors

What do you think? Could this work?

I think autofix would/could be nice but I'm not sure if it's trivial to do. For example given:

interface SomeInterface {
    foo: number; // <- Unused
}

export function fn1(options: SomeInterface) {
    if (options.foo) { // We could eliminate the interface member above, but then this becomes an error. Do we want to also fix and remove references to this field? How would that look in more complex situations?
        return 1;
    } else {
        return 2;
    }
}

[webpro]

What do you think? Could this work?

Not sure, mainly because of this:

In main function iterate over all functions

If you literally mean "all functions", I think they're not available there?

I think autofix would/could be nice but I'm not sure if it's trivial to do.

Indeed, looks like autofix seems out of range for safe removal for most (if not all) cases. Maybe there are some trivial cases we could fix, we might learn more along the way.

[SanderRonde]

If you literally mean "all functions", I think they're not available there?

Ah no I meant just all exported (so previously collected) functions.

Indeed, looks like autofix seems out of range for safe removal for most (if not all) cases. Maybe there are some trivial cases we could fix, we might learn more along the way.

Good point, will add some "todo: this could autofix using xyz" pointers along the way so it's easier to implement in the future once we go all the way and implement it all.

[webpro]

type and interface-member checking is def something that'd be easy for me to add

Def... not as easy as i expected... but got an alpha version working.

This should catch the original use case from the opening post (but obviously not the "runtime" case).

Installation:

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

Initial docs: https://42d094e9.knip.pages.dev/guides/type-members

I'll move this comment into a new issue if/when that makes sense.

Would be great if you could try this out, eager to hear about findings and false positives.

[SanderRonde]

Works like a charm!

A couple of comments:

• I noticed that if an interface is exported but never imported, it also isn't considered for this check. This means that the first example in on the docs page (with the dog) won't be reported if Dog is not also imported somewhere else. This might be slightly confusing to the user imo. While you do indeed mention "don't export interfaces just for Knip to check them", actually skipping the check would maybe be a bit much, given that Knip already reports on unused exports.
• Nitpick: would be better to also prefix the examples on the docs page with export to make it more clear that it's checking exported interfaces only

I think it would indeed be better to move the change you made to another issue and get it merged :)

[webpro]

Thanks for the quick feedback. Totally agree with your points. Although - at least for now - we'll have to resort to some "interesting" configuration to get the behavior you describe: https://09833a2d.knip.pages.dev/guides/type-members#closing-notes. It's in line with how Knip already works, but I also get it might be confusing. Moving to #910.

[SanderRonde]

Ahh well at least nice that there's a way to get it working anyways :)