[react-unused-props-and-state] add support for traversing function components

[shiftkey]

PR checklist

• Addresses an existing issue: fixes react-unused-props-and-state fails with SFC's #339
• New feature, bugfix, or enhancement
  • Includes tests
• Documentation update

Overview of change:

This is an attempt to support finding unused props in function components, based on the usage I've seen in the wild and figuring out how to traverse them properly. Fair warning: this is my first real contribution to anything related using the TypeScript AST in anger, so I want to caveat this with:

Jokes aside, I focused on these two general cases reported in #339.

First, an arrow function that returns a stateless function component. We can check more cases here, like a type literal being used rather than a type reference, because of the context that the presence of the React.SFC provides:

interface Props {
  children: JSX.Element | JSX.Element[];
  className: string;
}

const ButtonGroup: React.SFC<Props> = (props: Props) => {
  return (
    <div className={props.className}>
      {props.children}      
    </div>
  )
};

Second, function declarations which have a parameter matching the regex defined in config:

interface IProps {
	test: string;
	test2: string;
}

export function Test(props: IProps) {
	return <div>{ props.test }</div>;
}

I avoided being as flexible as possible because it might incorrectly traverse all functions in a module which have one parameter.

These changes should be additive to how class components are traversed. I didn't want to touch that flow despite the decent suite of tests covering it. Because I wanted these changes to be additive I introduced the callbackForFunctionComponentNodes to traverse the interesting parts of these arrow functions and function declarations and find any props usage.

TODO:

• look for React.FunctionComponent as well as React.SFC (SFC is deprecated in the latest @types/react package)

Is there anything you'd like reviewers to focus on?

Three things from the reviewers:

• general feedback about the approach and how it fits with the style of other linting rules
• being able to build a local NPM package and verify these changes inside a live project using tslint
  • cp dist/build/*.js ../other-project/node_modules/tslint-microsoft-contrib/ worked for me after b1e907a - is there a better way to do this?
• the core of this change was about how property expression worked, and I'd like to merge callbackForFunctionComponentNodes back into the main cb callback to be consistent but haven't quite got the traversal right. Let me know how strongly you'd like for this all to be in cb. fixed in 9977484

One thing from the community:

• I'd love to find additional test cases that aren't covered here which fit with the spirit of the rule. If you can run this PR up locally, add a test case with your snippet and whether it should pass or report errors, and leave a comment in this PR so I can follow up.

[IllusionMH]

Haven't chance to look at PR in details yet.
At first glance noticed that there are checks for React.SFC, however it is marked as deprecated in favor of React.FC. So this rules should check both of this types.

[shiftkey]

So this rules should check both of this types.

@IllusionMH thanks for the tip - I'll put that on my to-do list

[JoshuaKGoldberg]

This is a solid start to one of the most frequently requested improvements in this repo, thanks @shiftkey! 🙌

I think it can be generalized a bit to handle a few more cases.

[JoshuaKGoldberg]

general feedback about the approach and how it fits with the style of other linting rules

This seems like a pretty reasonable approach. This rule is an unusually crusty one, which seems inevitable given how vague the definition of what a "component" is. We're very fortunate to have a precedent of props interfaces being named *Props...

I think it could later be improved by also considering any arrow function, function declaration, or function expression that takes in a single object and returns anything assignable to React.ReactNode or React.ReactNodeArray. But that would be good discussion for a followup issue.

being able to build a local NPM package and verify these changes inside a live project using tslint

Yeah the build process for this package is a little wonky. 7.0 will, as a breaking change, remove a lot of older packaging that is no longer relevant, so it'll be possible to switch to in-place file compiles (in other words: no longer need to cp files before symlinking them). Until then, you could do something like:

cd ~/Code/repos/tslint-microsoft-contrib/src
tsc --outDir ~/Code/repos/some-other-project/node_modules/tslint-microsoft-contrib

...but you might have to fiddle around with compiler settings to get that to work.

Personally I just hand-edit what's in node_modules. 😅

[shiftkey]

I think this is ready for another round of review

[JoshuaKGoldberg]

Looks great! Just waiting on test cases for the other type names.

[JoshuaKGoldberg]

💯, this is excellent. Thanks @shiftkey!

We'll be cutting a release of tslint-microsoft-contrib this week to get this & some other changes out.

[IllusionMH]

PR is really great! I like updated and detailed test suite 👍

Looks like there only are couple of outdated comments in current code (1 has inline mention, and one in inside cb function) otherwise code looks good.

However there are two cases that I see regularly and are not supported now:

• Props destructuring inside function
• "Renames" in named parameters (see example below)

@shiftkey would you like to look at these cases in scope this PR or should they be handled in separate PRs?

Code example: "Renames" in named parameters for render components

In this case I have to "rename" labelComponent to Label, otherwise React will treat it as regular HTML tag.

import React from 'react';

type MetaFields = {
    label: string;
    value: string;
};

interface Props {
    items: ReadonlyArray<MetaFields>;
    labelComponent: React.ComponentType<{ text: string }>; // incorrectly marked as unused
}

export const Meta: React.FC<Props> = ({ items, labelComponent: Label }: Props) => (
    <ul>
        {items.map(({ label, value }) => (
            <li key={label}>
                <Label text={label} />
                <span>{value}</span>
            </li>
        ))}
    </ul>
);

[shiftkey]

@IllusionMH lets open a fresh issue or two to capture these other gaps - I'd rather not hold up getting this into an update for the new cases it currently covers.

[IllusionMH]

@shiftkey OK, I'll create separate issues for mentioned cases.

Thanks you for this contribution!