Fix false positive in no-unused-proptype

[jseminck]

This related to the following issue: #1183. It also seems to fix: #1135

Note: I am not very experienced with the code base. I took a look at this bug to learn a bit more about the code. Therefore I am not sure if my solution is the right solution - but in the worst case perhaps it can help someone else figure out the real issue if it is completely wrong. All the tests continue to pass and I added tests that were failing before my changes.

I tracked the issue down to components.list() function.

• It seems that both these callbacks are somehow evaluated to true for the condition on line 101: if (component) { -- this part seems a bit weird to me? Why are these functions components?
• The existing code then assigned the list of usedPropTypes for that "component" to usedPropTypes[this._getId(component.node)]

But because of the direct assigning, the second time the code here is executed it will overwrite the values that were stored the first time.

So with the example false positive we have:

    const callback = () => {
        props.a(props.b);
    };

    const anotherCallback = () => {};

• When the loop function detects callback, it then assigns prop a and prop b to usedPropTypes[myComponentId]
• When the loop function detects anotherCallback, it then overwrites the existing array of usedPropTypes and replaces it with an empty list (since no props are used in anotherCallback).

This also explains why the other examples work:

    const callback = () => {
        props.a(props.b);
    };

In here there is no second callback that overwrites the first array.

    const anotherCallback = () => {};

    const callback = () => {
        props.a(props.b);
    };

In here we overwrite the first callback that assigns an empty array with an array that contains the used props.

My fix includes not a direct assignment, but adding the detected propTypes to the existing list if a list already exists.

[ljharb]

Thanks, your test cases are great.

[ljharb]

s/var/const/g

[jseminck]

Unfortunately it's not so simple in this file. There are many vars that cannot be changed to const.

Personally I also feel like a file should either use only var, or only let + const. Using a mix of var, let and const is meh.

If you want, I can make some separate PR and transform some of the code-base to ES6? I have used Lebab before (https://github.com/lebab/lebab). I just need to figure out what ES6 features are supported and which aren't. Although we could just start with getting rid of var completely?

[ljharb]

I agree; we should only use const and let.

Regardless, we shouldn't add new instances of var - a mix is preferable to that.

I'd prefer manual modification to a codemod.