no-unused-prop-types false positive when prop is used in function

[zimme]

Here's a simplified example.
We're using flow, but I guess this will fail when using propTypes also.

type SomeComponentPropsType = {
  onSelectedPlace(query: any, place: any) => void, 
};

// This has been simplified.
const onSelectPlace = component => (query, place) => {
  component.props.onSelectPlace(query, place);
};

// Changing the above to the following doesn't work either.
const onSelectPlace = ({ props }: { props: SomeComponentPropsType }) => (query, place) => {
  props.onSelectPlace(query, place);
}

class SomeComponent extends Component {
    props: SomeComponentPropsType;

    render() {
      return (<button onChange={onSelectPlace(this)}>Select<button>);
    }
}

[ljharb]

It seems like a very strange pattern to pass this around, as opposed to passing only the information that onSelectPlace actually needs.

There's a limit to how much a linter can follow values being passed around, and passing around this seems certain to defeat many forms of static analysis, as well as being a subpar thing for maintainability.

[zimme]

Yes, but that's how the codebase on the project I've just recently started working on looks. I'm setting up linting rules for it and I ran into this problem and thought I should at least report it.

I get that following this around might be too much to ask for, but maybe the deconstructuring case could be supported as you provide the typing info on the parameter list.

However I have no idea how the linting works under the hood as I haven't looked at the code, yet, and it might not be possible at all.

Anyways, I'll probably just refactor things a bit as it's an unusual pattern.

[zimme]

Just a a side note, I believe the pattern was used to workaround binding of class methods.
But this is easier now with class property initializers and arrow functions.

[ljharb]

Binding of class methods in the constructor is the proper and idiomatic way to handle these things in React, fwiw. Using arrow functions as class properties is actually much less efficient, because it creates the entire function N times - whereas a constructor-bound prototype method is created once, and the only thing created N times is a tiny wrapper function (the .bind) that calls into the shared, optimizable method.

[zimme]

Would that hold true for using this.onSelectPlace.bind(this) inline, also? I'm thinking about the new binding syntax here, ::this.onSelectPlace.

Thank you for the insight, I didn't realize the impact of arrow function as class properties.

[ljharb]

Yes, the bind operator is identical to using .bind, which means you would not want to do it inline.

The best is to do this.foo = this.foo.bind(this)/this.foo = ::this.foo; in your constructor, or to add a class property like foo = this.foo.bind(this)/foo = ::this.foo.

[zimme]

Anyways, if you really feel that this case shouldn't be supported then feel free to close this. At least the conversation is on record and anyone else running into this will know that there's a better way to solve this.

[ljharb]

The issue should stay open in case there is a way to implement it :-) sorry for going offtopic with the code style discussion.

[robwise]

Using arrow functions as class properties is actually much less efficient, because it creates the entire function N times

Theoretically, I see your logic, but do you have any numbers on this? I can't see this possibly making a noticeable difference unless you're rendering thousands of instances of the same component.

Anyway, back to the OP, I'm having this issue without the use of this. My example is strange (using fat arrow function for no reason), but the real example was too domain-specific to be useful and this is definitely not expected behavior.

• So far, the times it happens in my app all have in common that I use the prop inside of a fat arrow function (either a handler like this example or stuff like mapping over an array).
• If you switch the order of the buttons, then it's the onClick prop that raises the error.

// @flow
import React from 'react';

type Props = {
  onMouseOver: Function, // 'onMouseOver' PropType is defined but prop is never used
  onClick: Function,
};

const MyComponent = (props: Props) => (
  <div>
    <button onMouseOver={() => props.onMouseOver()} />
    <button onClick={() => props.onClick()} />
  </div>
);

export default MyComponent;

[ljharb]

@robwise "numbers" would all be either app-specific, or microoptimizations that aren't worth calculating :-) It may indeed not make a noticeable difference - but I find it both theoretically more performant and actually more correct and maintainable. ¯_(ツ)_/¯

I'm assuming that if you convert the Flow back to propTypes, that everything's fine - if so, then this is a bug with the plugin's Flow parsing. Could you also try using { onMouseOver, onClick } instead of props?

[robwise]

See, I feel the opposite about maintainability, because you have to remember to add the binding in the constructor for a method that lives somewhere else in the file, and then if you delete said method, you have to remember to go find its binding and remove it again. With fat arrow class properties, when you delete your method, you're done.

I'm assuming that if you convert the Flow back to propTypes, that everything's fine

If I convert to use PropTypes instead of Flow, same problem:

const MyComponent = (props) => (
  <div>
    <button onClick={() => props.onClick()} />
    <button onMouseOver={() => props.onMouseOver()} />
  </div>
);

MyComponent.propTypes = {
  onMouseOver: React.PropTypes.func.isRequired,
  onClick: React.PropTypes.func.isRequired, // 'onClick' PropType is defined but prop is never used
};

export default MyComponent;

Could you also try using { onMouseOver, onClick } instead of props?

There's no error if I destructure:

const MyComponent = ({ onMouseOver, onClick }) => (
  <div>
    <button onClick={() => onClick()} />
    <button onMouseOver={() => onMouseOver()} />
  </div>
);

MyComponent.propTypes = {
  onMouseOver: React.PropTypes.func.isRequired,
  onClick: React.PropTypes.func.isRequired,
};

export default MyComponent;