Extract duplicate code between prop-types and no-unused-prop-types #1

[jseminck]

Follow-up for: #1526

Basically in prop-types and no-unused-prop-types there is quite a bit of duplicate code. I tried to do a HUGE refactoring to remove a lot of it but it turned out... well... not good 😄 So maybe smaller PRs is a better idea.

Basically made the "are we in a lifecycle method" detection similar between prop-types and no-unused-prop-types. Both rules were doing it differently but the results were the same. There was also duplicated methods like inComponentWillReceiveProps.

There should be some new cases detected now, e.g. in prop-types rule: nextProps should be considered not only for componentWillReceiveProps, but also for shouldComponentUpdate.

On that note, I noticed that prevProps is not supported for prop-types rule, with a lifecycle method of componentDidUpdate for example. There's some further refactoring needed to align the two rules more to get that properly working which I'd like to tackle in a PR after this.

[ljharb]

Just in the beginning, I'm a little concerned about the conflation of "is in a lifecycle method" with "is in the constructor" - could we start by keeping those separate, so the refactor is more 1-to-1?

[ljharb]

is this necessarily equivalent? context.getScope() won't be the same as node.parent.parent, necessarily.

[jseminck]

Thanks for feedback. I'm at a conference for the next 2 days, but I will make "is in constructor" separately and deeper investigate the differences isNodeALifeCycleMethod(node.parent.parent) vs astUtil.inLifeCycleMethod(context), which I do believe achieve the same thing, but technically they are doing something different.

[ljharb]

LGTM! For posterity, could you reply to my comments with a link to the lines of code for the test cases in question?

[ljharb]

which test case exposes this missing inConstructor check?

[jseminck]

Note that the old isInLifeCycleMethod function already was checking whether we are in constructor: node.kind === 'constructor'. Now it is split into inConstructor and inLifecycleMethod so the code is the same between this rule and prop-types.

In this case, the following two tests will fail when removing this line:

• https://github.com/yannickcr/eslint-plugin-react/blob/master/tests/lib/rules/no-unused-prop-types.js#L2037
• https://github.com/yannickcr/eslint-plugin-react/blob/master/tests/lib/rules/no-unused-prop-types.js#L1213

[ljharb]

also here, which test case exposes the need for this check?

[jseminck]

Same as above: Note that the old isInLifeCycleMethod function already was checking whether we are in constructor: node.kind === 'constructor'. Now it is split into inConstructor and inLifecycleMethod so the code is the same between this rule and prop-types.

However, this case seems to be untested. 😞 In master we can remove this whole block and the tests still pass.

&& ( utils.getParentStatelessComponent() || isInLifeCycleMethod(node))

[ljharb]

also here

[jseminck]

Same as above.

Fails when removing inConstructor:

• https://github.com/yannickcr/eslint-plugin-react/blob/master/tests/lib/rules/no-unused-prop-types.js#L1228

When removing the inLifecycleMethod then 5 tests fail (2 for componentWillReceiveProps, 1 for shouldComponentUpdate and 2 for componentDidUpdate)

I suppose that we can also test https://reactjs.org/docs/react-component.html#componentwillupdate which also has nextProps as the first argument.

[ljharb]

also, which test cases warrant this change from just checking willReceiveProps, to checking all the lifecycle methods?

[jseminck]

This is missing tests! Indeed we will now check more cases than just componentWillReceiveProps

[ljharb]

@jseminck @alexzherdev is this PR still useful, now that #1911 and friends have landed?

[alexzherdev]

It seems like what this PR contains has been achieved in spirit: there is now a single place that defines inConstructor and inLifecycleMethod, which is lib/util/usedPropTypes.js.

[ljharb]

Are there test cases here we could salvage?