Eslint-plugin-react: react/prop-types - is missing in props validation - false/positive when using inner function

[jkrawczyk]

Hi, I came across the issue with react/prop-types rule.

import React from 'react';
import PropTypes from "prop-types";

const DisplayName = (props) => {
    const getNameDiv = () => {
        return <div>{props.name}</div>;
    };

    return getNameDiv();
};

DisplayName.propTypes = {
    name: PropTypes.string.isRequired,
};

gives me warning 'name' is missing in props validation react/prop-types. Same thing happens if I rewrite function to function getNameDiv() {...}

After inlining getNameDiv there is no warning:

import React from 'react';
import PropTypes from "prop-types";

const DisplayName = (props) => {
    return <div>{props.name}</div>;
};

DisplayName.propTypes = {
    name: PropTypes.string.isRequired,
};

I was looking for similar issues but I didn't find any. Maybe https://github.com/yannickcr/eslint-plugin-react/pull/1605 is connected?

I was using eslint-plugin-react: 7.11.1 but it's the same for 7.13.3.

Thanks!

[ljharb]

In this case, getNameDiv is actually being detected as a component (which it should be; prefer components to render functions).

I'm not sure how we could rule out getNameDiv as being a component without ruling out many valid use cases.

[jkrawczyk]

This is my use case:

import React from "react";
import PropTypes from "prop-types";

const DisplayNames = (props) => {
    const namesTable = () => {
        return <table className="table">
            <thead>
            <tr>
                <th>Name</th>
            </tr>
            </thead>
            <tbody>
            {
                props.names.map((name, index) =>
                    <tr key={index}>
                        <td>{name}</td>
                    </tr>
                )
            }
            </tbody>
        </table>;
    };

    const noNames = () => {
        return <span>No names</span>;
    };

    return props.names.length > 0 ? namesTable() : noNames();
};

DisplayNames.propTypes = {
    names: PropTypes.array.isRequired,
};

I didn't want to extract additional components because for me it was integral part of the component I was creating. Function was extracted just for better readability.

I understand that this use case may be hard to recognise. I suppose I will leave it and just disable the rule for the file.

Thank you.

[ljharb]

Certainly that’s subjective; personally I’d find it much more readable for noNames to be inline jsx, and for namesTable to be a separate component.

Let’s leave this open in case there’s a way to handle it.

[jkrawczyk]

Certainly! 😃Thanks!

[dvyue]

I am having the same issue with inner function. My use case is overriding a component in material-table. I didn't want to extract the two-liner inner function into a separate component.

[ljharb]

Why not?

[jaydunning]

Because the purpose of PropTypes is to enable proper use of a module's interface, not to enforce a module implementation structure. This limitation is particularly burdensome when working with 3rd-party components where a one-line change turns into a massive refactoring to eliminate this error.

[ljharb]

@jaydunning you only lint your own code, not third-party code; i'm not clear on why this would be burdensome.

[jaydunning]

So I had 30 minutes to correct a bug at line 209 of material-ui-treeview. It's a simple, one-file module with a permissive license, so I copied it into my code base and made the (trivial) change, then spent the next 19 minutes puzzling over how to remove this lint error before I disabled the rule for the module, then 5 minutes on the comment above. I'm referencing the specific file because I think it may provide a good concrete example of what I meant by "burdensome". The location responsible for the errors (line 152) is a recursive function within a component; the function itself behaving as a component but needing its closure, and I couldn't think of a quick way to refactor it to satisfy this rule. Further, it seemed to me that the design was suited to purpose, and any refactoring to eliminate the rule error would degrade the module's design. This may be an exception case, and perhaps disabling the rule is the expected remedy, and I BTW agree that OP's example is not ideal and think that detection of the anti-pattern is a good thing, but I also believe there's a critical distinction between interface and implementation, and that the prop-types rule should be about interface, not implementation, because PropTypes is about interface, not implementation.

[ackvf]

Originally from https://stackoverflow.com/a/60510524/985454

I have discovered a strange behaviour. The only difference is in the return statement:

Now also the bar in propTypes is uncommented:

I think the problem is because the myFunc looks like a functional component and that eslint is picking it up wrongly.

Also, this is rather funny, if you simply rename the props to anything else, eslint goes silent :)

[ackvf]

@ljharb React itself differentiates that myFunc is a function and MyFunc is a component, so why not assume the same?

myFunc can't be used as a component anyway, this code is invalid <myFunc />.

[ljharb]

@ackvf react does runtime introspection; a linter can not.

[ackvf]

@ljharb myFunc vs MyFunc is known at compile time or am I missing something?

[ljharb]

That is true; our component detection should ideally de-weight non-PascalCase-named functions. However, you shouldn’t be calling a function to return jsx, it should be a component :-)

[ackvf]

@ljharb In this case yes, but there is a valid use case for it, a component factory, consider:

  const heading = level => {
    const Tag = `h${level}`
    return <Tag/>
  }

_(though this is not really how the factory should be used)_

and given the previous analysis, I believe eslint would think heading is a JSX component and would warn about missing propTypes.

---

or

render () {
  const maybeDisplayDiv = () => condition && <div>Visible</div>
  return maybeDisplayDiv()
}

[ackvf]

Anyway, since lowerCase name for a component is not supported by React, eslint should probably do the same and assume that any lowerCase is not a component, but a function.

Also, can you please explain to me what's happening in the last gif when props is recognized, but prop not?

[ljharb]

That's an element factory (ie, a component itself); a component factory would return Tag and be used inside jsx that way.

In that last gif, I assume the rule is using the argument name of "props" to convince itself it's a component. A PR with test cases that tweaks the component detection would be welcome.

[stickfigure]

you shouldn’t be calling a function to return jsx, it should be a component

Not always. Making a component involves quite a lot of boilerplate; sometimes within a component it's easier to use a simple inline function. I find it's much better from a readability perspective to replace trees of ternary statements with a function containing a series of if/then statements.

This is pretty gross, especially with more conditions:

const thing = cond1 ? cond2 ? <Comp1/> : <Comp2/> : <Comp3/>;
return <div> ... a bunch of other stuff ... {thing}</div>;

This is more readable:

function thing() {
    if (cond1)
        if (cond2)
            return <Comp1/>;
        else
            return <Comp2/>;
     else
         return <Comp3/>;
}
return <div> ... a bunch of other stuff ... {thing()}</div>;

One of the nice things about working with jsx/tsx is that "it's just javascript". This lint rule rejects a common javascript idiom.

[ljharb]

What's even more readable would be:

let thing;
if (cond1) {
  if (cond2)
    thing = <Comp1/>;
  } else {
    thing = <Comp2/>;
  }
} else {
  thing = <Comp3/>;
}
return <div> ... a bunch of other stuff ... {thing}</div>;

There's no value in adding an additional function call there.

[stickfigure]

That style of code is strongly frowned upon by functional programmers.

[ljharb]

However, FP style in the way you're referring isn't what's idiomatic or common for JS, a multi-paradigm language.

[stickfigure]

As you say, JS is a multi-paradigm language. Functional style is idiomatic and common for a very large community of JS programmers. React promotes it.

This lint rule does not keep with the 'multi-paradigm' philosophy.

[ljharb]

React in no way promotes creating functions in the render path for immediate invocation in the render; that's just silly and wasteful.

Clearly the component detection shouldn't identify as a component a function with a lowercase name that's not used in a jsx context. I'd be happy to review a PR that improved component detection.

[nader2560]

@ljharb React itself differentiates that myFunc is a function and MyFunc is a component, so why not assume the same?

myFunc can't be used as a component anyway, this code is invalid <myFunc />.

Yes but Eslint is viewing it as such in other words this should solve the issue
myFunc.propTypes = { bar: PropTypes.string.isRequired }

[ljharb]

While that's true, React won't be able to render it correctly - it's flat out invalid code. jsx custom component elements must start with a capital letter.

[otaviogui]

Hello, This post can resolve your problem.

Look this:
http://www.hackingwithreact.com/read/1/41/how-to-add-react-component-prop-validation-in-minutes

but he is using it in a deprecated way, to solve this.

Make this like:

function YourFunction ({params}) {

     return(
       {params}
      );

}

YourFunction.propTypes = {
params: PropTypes.object.isRequired,
};

[pascalduez]

Seems to be trigggered as soon as there's not JSX returned? It just popped out of the blue for me...

// @flow

import * as React from 'react'

type Props = {
  tagName?: string,
  children: React.Node,
}

function Base({ tagName = 'div', children }: Props) {
  return React.createElement(tagName, {}, children)
}

Okay one could:

function Base({ tagName: Component = 'div', children }: Props) {
  return <Component>{children]</Component>
}

But still...