Handle TSTypeReference in no-unused-prop-types

[niik]

👋 from GitHub Desktop land!

We're currently upgrading to TypeScript 4.5 (desktop/desktop#13768) and while doing that I wanted to start using the react/no-unused-props-type rule instead of the react-unused-props-and-state from the obsolete tslint-microsoft-contrib package.

Unfortunately I couldn't get the rule to produce any errors. I put up a minimal reproduction repository at https://github.com/niik/no-unused-prop-types-repro which illustrates the problem. To test it just clone and run yarn lint

Doing so very haphazardly troubleshooting I noticed that when encountering our prop types in propTypes.js they were of type TSTypeReference which was not being handled in the switch case. Spotting that DeclarePropTypesForTSTypeAnnotation seemed to be able to handle both TSTypeAnnotation and TSTypeReference internally I simply tried adding it to the switch and lo-and-behold the rule just started working.

Now, obviously I have no idea what I'm doing here so I'm happy to adjust my approach given some guidance from someone who understands eslint/ts-parser internals but running my minimal reproduction repository with my branch (https://github.com/niik/no-unused-prop-types-repro/tree/with-ts-type-reference-fix) reports the error correctly.

The fix looks great, but if we don't have a regression test, it will guaranteed be broken again in the future :-)

Your repro looks pretty small, so it should be straightforward to adapt it into a test case. Want to give it a shot?

Your repro looks pretty small, so it should be straightforward to adapt it into a test case. Want to give it a shot?

I'd be happy to but I'm not sure how I should go about it. There's already plenty of tests cases that cover this scenario in the no-unused-props-type rule tests (like https://github.com/yannickcr/eslint-plugin-react/blob/74634488e8682bc74e8d93e03b5ad70cf47551a9/tests/lib/rules/no-unused-prop-types.js#L5620-L5635). These tests will presumably break once this project upgrades to TS 4.5 (or whatever dependency it is that ends up leveraging the TS compiler types). Pointers on how such a test could be written would be much appreciated.

ahhh gotcha, it's the TS version specifically?

What version of the TS eslint parser are you using?

Looks like v5.10, and we're already testing on ^5. I'd hope they didn't change the name of the parse nodes in a minor version, but if they had I'd have expected to see failures sooner.

Codecov Report

Merging #3195 (36482a3) into master (7463448) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3195   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files         120      120           
  Lines        8301     8302    +1     
  Branches     2987     2988    +1     
=======================================
+ Hits         8096     8097    +1     
  Misses        205      205           

Impacted Files	Coverage Δ
lib/util/propTypes.js	97.42% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7463448...36482a3. Read the comment docs.

I reran the passing tests on master from 3 days ago, and they continued to pass: https://github.com/yannickcr/eslint-plugin-react/actions/runs/1767687068 - so we do need some kind of new coverage, or possibly, to upgrade to TS 4.5.

That would imply it's a breaking change in TS tho.

So, I couldn't make this test fail locally. Then, I installed different versions of TS and the TS eslint parser, and was able to make it fail. Then, I couldn't get it to NOT fail (absent the fix).

So, I guess we'll land it, but something weird is going on.