Skip to content

Conversation

@ulrichstark
Copy link

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • Test
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • I have added a convincing reason for adding this feature, if necessary

Other information

A pull request of a TypeScript type only version no-unused-prop-types is welcome.
#85 (comment)

@vercel
Copy link

vercel bot commented Jul 21, 2025

@ulrichstark is attempting to deploy a commit to the Rel1cx's projects Team on Vercel.

A member of the Team first needs to authorize it.

@ulrichstark
Copy link
Author

Hey @Rel1cx, I'm already making better progress than expected on this rule. As you can see, four basic invalid and four basic valid tests are already passing. Before continuing I wanted to confirm that the direction I'm going matches your expectation of this rule.

The most important question right now is if I should count prop keys as used if they are destructered/accessed from the props object, but NOT used after that (in JSX for example). I would decide for no, because that case is already covered by https://eslint.org/docs/latest/rules/no-unused-vars. See my valid tests for reference. They currently pass because destructuring from the props object is enough to be counted as used.

Some other decisions I made so far:

  • Rule features are TSC and EXP
  • Added my rule to all and x config
  • Added my rule to recommended config, but commented out

If you notice anything incorrect in my current changes, feel free to point that out or change it to your desire.

@Rel1cx
Copy link
Owner

Rel1cx commented Jul 23, 2025

I would decide for no, because that case is already covered by https://eslint.org/docs/latest/rules/no-unused-vars

You are right. This aligns with our philosophy: Each rule should have a single purpose. Make multiple rules work together to achieve more complex behaviors.

So far so good, Thanks for the amazing work!

@vercel
Copy link

vercel bot commented Jul 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eslint-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 2, 2025 5:18pm

Signed-off-by: Ulrich Stark <github@ustark.de>
Signed-off-by: REL1CX <dokimondex@gmail.com>
@ulrichstark
Copy link
Author

Now there are 38 passing tests and I'm very happy with the progress so far. Some questions:

  1. How mature and complete should the rule become to get this PR merged?
  2. How much should I focus on class components?
  3. How would you handle exported props interfaces/types? Ignore them completely because props not used by the component could be used by some external module? Or just ignore the fact that it's exported and might risk false positives?
  4. Any features or test coverage wishes?

@ulrichstark ulrichstark marked this pull request as ready for review July 28, 2025 17:51
@ulrichstark
Copy link
Author

I think we are ready for a first round of review on this PR. If the property is declared in a different file than the component, then I don't report it and actually can't even if I want because TypeScript knows the AST node, but ESlint doesn't as far as I understand. My focus was to report very conservatively to avoid false positives. Type information is required for it to work. Class components aren't supported by this rule – should I add support for it? And please double check all the changes in docs and plugin presets and there might be something missing as this is my first contribution to your project.

Add some test cases that are closer to real-world scenarios

Signed-off-by: REL1CX <dokimondex@gmail.com>
@Rel1cx
Copy link
Owner

Rel1cx commented Jul 31, 2025

Now there are 38 passing tests and I'm very happy with the progress so far. Some questions:

  1. How mature and complete should the rule become to get this PR merged?
  2. How much should I focus on class components?
  3. How would you handle exported props interfaces/types? Ignore them completely because props not used by the component could be used by some external module? Or just ignore the fact that it's exported and might risk false positives?
  4. Any features or test coverage wishes?
  1. I believe the current maturity and completeness of this rule are sufficient for the PR to be merged.

  2. There is no need to support class components.

  3. I think we should skip checking exported props interfaces/types. This is because correctly handling props type definitions and their usage across files would require multi-file static analysis, which is not ideal under ESLint’s single-file architecture. I am also keeping an eye on the exploration of multi-file static analysis in TS linters, such as @ArnaudBarre/tsl/issues/2. If there is a need for such functionality, it might be more suitable to implement it in those tools.

  4. I added additional features and test coverage in no-unused-props.spec.ts, mainly focusing on how this rule interacts with PropsWithChildren, forwardRef, and ref as a prop.

Signed-off-by: REL1CX <dokimondex@gmail.com>
Signed-off-by: REL1CX <dokimondex@gmail.com>
Signed-off-by: REL1CX <dokimondex@gmail.com>
@ulrichstark ulrichstark requested a review from Rel1cx August 2, 2025 15:20
Copy link
Owner

@Rel1cx Rel1cx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Rel1cx Rel1cx merged commit 99cd049 into Rel1cx:2.0.0-beta Aug 2, 2025
7 checks passed
@ulrichstark ulrichstark deleted the no-unused-props branch August 2, 2025 18:37
@ulrichstark
Copy link
Author

Sorry for bothering, but I expected my rule to be available in the published version 2.0.0-beta.18 of @eslint-react/eslint-plugin but it isn't. Manually turning it on in the rules object of my eslint.config.js gives me following error:

TypeError: Key "rules": Key "@eslint-react/no-unused-props": Could not find "no-unused-props" in plugin "@eslint-react"

Is that somehow expected by you or have we missed something in this PR?

@Rel1cx
Copy link
Owner

Rel1cx commented Aug 2, 2025

Sorry for bothering, but I expected my rule to be available in the published version 2.0.0-beta.18 of @eslint-react/eslint-plugin but it isn't. Manually turning it on in the rules object of my eslint.config.js gives me following error:

TypeError: Key "rules": Key "@eslint-react/no-unused-props": Could not find "no-unused-props" in plugin "@eslint-react"

Is that somehow expected by you or have we missed something in this PR?

The problem is not with this PR, but with the outdated CI configuration for publishing. Thank you for your timely feedback, it helped me discover the issue.

@Rel1cx
Copy link
Owner

Rel1cx commented Aug 2, 2025

This rule has now been released in 2.0.0-beta.19.

@Rel1cx Rel1cx added Status: Released The issue has been released Type: New Rule Introduce a new rule labels Aug 2, 2025
@ulrichstark
Copy link
Author

ulrichstark commented Aug 2, 2025

Yes thanks!
I can confirm that my rule is included and working as expected in 2.0.0-beta.19.

What would it take to include my rule in the recommended-type-checked preset? I think a lot of developers would benefit from this rule and it already should be very low on false positives.

What do you think about changing the message from "Prop hello is declared but never used" to "Property" instead of "Prop". I think I like "Property" more with how it looks in the IDE contrary to what I coded in this PR haha.

@Rel1cx
Copy link
Owner

Rel1cx commented Aug 11, 2025

Yes thanks! I can confirm that my rule is included and working as expected in 2.0.0-beta.19.

What would it take to include my rule in the recommended-type-checked preset? I think a lot of developers would benefit from this rule and it already should be very low on false positives.

What do you think about changing the message from "Prop hello is declared but never used" to "Property" instead of "Prop". I think I like "Property" more with how it looks in the IDE contrary to what I coded in this PR haha.

Sorry for the delayed reply. I’m considering adding this rule to the recommended-type-checked preset. Before doing that, I’ll trial it across a few open-source projects to see how it behaves at scale. A very low false-positive rate is great (especially for a newly implemented rule), but it’s only one evaluation factor. Another important factor is whether it creates too many visual noise while writing React components and distracts developers.

Regarding the wording, we’ve thought about this before. After some research, we believe it’s best to stay aligned with the terminology used on https://react.dev, so I’d prefer to keep the current "prop" (consistent with react.dev) rather than switching to "property".

@ulrichstark
Copy link
Author

Yes thanks! I can confirm that my rule is included and working as expected in 2.0.0-beta.19.
What would it take to include my rule in the recommended-type-checked preset? I think a lot of developers would benefit from this rule and it already should be very low on false positives.
What do you think about changing the message from "Prop hello is declared but never used" to "Property" instead of "Prop". I think I like "Property" more with how it looks in the IDE contrary to what I coded in this PR haha.

Sorry for the delayed reply. I’m considering adding this rule to the recommended-type-checked preset. Before doing that, I’ll trial it across a few open-source projects to see how it behaves at scale. A very low false-positive rate is great (especially for a newly implemented rule), but it’s only one evaluation factor. Another important factor is whether it creates too many visual noise while writing React components and distracts developers.

Regarding the wording, we’ve thought about this before. After some research, we believe it’s best to stay aligned with the terminology used on https://react.dev, so I’d prefer to keep the current "prop" (consistent with react.dev) rather than switching to "property".

Sounds great! Let me know if you need any thoughts from me and feel free to assign me to future issues related to this rule. If you want a sponsor here on GitHub, please enable GitHub sponsorships and I will be your first sponsor. Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Released The issue has been released Type: New Rule Introduce a new rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants