Non-component functions that return JSX

[perrin4869]

react-router allows you to override the createElement function when rendering a route component. For example:

<Router history={browserHistory} createElement={createElement}>{...}</Router>

const createElement = (Component, props) => (
  props.route.dependencies = props.route.dependencies || {};
  return React.createElement(Component, { ...props, ...props.route.dependencies });
);

This createElement function is not a component (a component is a pure function of the props, but in this case it has two parameters, the component and the props), however, eslint recognizes it as one and demands that the propTypes be defined. Maybe the component recognition as specified here: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/display-name.md can be narrowed down to functions with only one parameter.

Edit: the error I get is 'route' is missing in props validation (react/prop-types)

[perrin4869]

Actually, stateless functional components can get a second context component... not sure how to solve this...

[yannickcr]

I was not aware of this functionality of react-router, it's a very special case. Right now I don't see how to handle this but I will think about it.

[mjackson]

FWIW react-motion also allows you to specify a children prop that returns React elements. We're currently working on moving the react-router API even more in this direction, so we need a fix here.

I'd suggest we stop treating any function that returns JSX as a stateless functional component unless we can also prove that someone is using it in a call to createElement somewhere. Since I suspect the latter isn't really feasible, I'd suggest we stop treating functions that return JSX the same as if they were components. They are not the same.

[ljharb]

It's not just any function that returns JSX - it's functions that return JSX and take a "props" (or destructured) first argument, and an optional "context" second argument.

How else would you propose detecting SFCs so that rules can be applied?

[mjackson]

That's not true. I just triggered the react/display-name warning on an anonymous function with a blank argument list.

The only way you can definitively tell that the user intends to use a function as a SFC is to see if they ever use it as the first arg to React.createElement.

[ryanflorence]

How else would you propose detecting SFCs so that rules can be applied?

I think you just have to check if it's anonymous or not. If anonymous, it's not a component.

// don't warn
<Blah stuff={({ foo }) => ()}/>

// warn
const Thing = ({ foo }) => ()

Note, maybe anonymous means something else, another way to phrase this is "is the function we think is a SFC assigned to a variable or not?"

[ljharb]

Yeah, all arrows are anonymous, but I think you're saying, a "maybe SFC" (the current rubric, which as @mjackson points out might not be as smart as I think it should be, including the fact that SFCs don't have to use props at all) that is also exported or assigned to an identifier - as opposed to one that's passed directly as an argument to something that's not React.createElement.

I think regardless that the metric for detecting SFCs needs to be as accurate as possible, and it's highly likely that it's both not, and inconsistently applied. However, it's far better imo to have an overly-active lint rule that can be overridden with comments, than to have one that misses otherwise lintable errors.

[ryanflorence]

What's wrong w/ keeping your current metric and adding "assigned to a variable"? Is there ever a case where you have a SFC that isn't assigned to a variable?

[ljharb]

export default () => <div />

[lencioni]

@ljharb

It's not just any function that returns JSX - it's functions that return JSX and take a "props" (or destructured) first argument, and an optional "context" second argument.

Not all SFCs take props though, right?

[ljharb]

@lencioni yes, you're right - i mentioned that in #512 (comment)

[ryanflorence]

assigned to variable OR default export

[ljharb]

@ryanflorence if that includes const foo = { bar: () => <div /> } under "assigned to variable" i totally agree, which is what I said in #512 (comment) :-)

[ryanflorence]

Sounds good.

However, it's far better imo to have an overly-active lint rule that can be overridden with comments, than to have one that misses otherwise lintable errors.

Don't forget that many people like myself will just disable the rule when it's this noisy around false positives (I use these react-motion render props all over the place) and the end effect is no linting for any propTypes, or if they're a beginner be totally confused, so I tend to think the other direction :)

[mjackson]

However, it's far better imo to have an overly-active lint rule that can be overridden with comments, than to have one that misses otherwise lintable errors.

I'm just worried that other people are going to stumble on this error and think "man, React is hard" just because they made a function that returns some JSX and they're using the "recommended" lint setting. Can we perhaps separate out this functionality into an opt-in sfc-display-name rule?

[yannickcr]

I try to reduce false positive/negative when detecting React Components, but it is particularly hard with SFC since they are simple functions and since the linting is made file by file we don't know where/how it will be used.

Right now I think we are not too bad at detecting SFC since we have covered most of the common patterns used by the community. Of course there is still some edge cases (like the pattern used here by react-router for createElement) that can be problematic. But in this case it is not unfixable imho.

@mjackson:

Can we perhaps separate out this functionality into an opt-in sfc-display-name rule?

Some people have already asked for an ignore-sfc option for rules like display-name or prop-types. It could be something nice to have.

FWIW react-motion also allows you to specify a children prop that returns React elements. We're currently working on moving the react-router API even more in this direction, so we need a fix here.

Beside the original case of this issue, did you encountered other problematic patterns with react-motion or the future react-router API?

[mjackson]

I first encountered this error this afternoon while working on the <Media render> API for react-media. The rule was complaining about this test specifically.

We will have an identical API in the next release of react-router that renders something when a particular route matches. Unfortunately, for now I just had to disable react/display-name entirely because it's just too aggressive.

[yannickcr]

@mjackson

The rule was complaining about this test specifically.

Ok, in this was definitely a bug. I made a fix on component detection for this particular case, so it should be better in next release (does not fix the original problem of this issue though).

[mjackson]

Thanks, @yannickcr. I assume you're talking about the work you did in 70bf28d. Just to be clear, how does that work alter the current behavior?

[yannickcr]

Yes, the only relevant change here is the addition of components.add(node, 0);

The second parameter is the "confidence", when searching for components we rank them with confidence levels:

• 2: Component
• 1: Maybe a component, but we're not sure
• 0: "ban" the node, it will not be considered as a component even if we have a confidence level of 2 for it

This is due to the fact that the AST is traversed only one time by ESLint so we need to be sure to store information about components (node, propTypes, etc.) even if we are not quite sure that they are components yet.

At the end we only validate the components with a confidence level of 2.

With this change if I cannot find a component from an (Arrow)FunctionExpression or if it is in a JSXExpressionContainer (which is the case in your pattern) then we "ban" the node, so this way when later we see that it returns some JSX (confidence 2) we do not still consider it as a component, so it will not be validated.

(Hope I was clear, the Components.js file could use some serious refactoring to be more readable)

[nene]

I'll add my vote for an option to disable prop-types for Stateless Functional Components. For those who aren't using them, it would easily eliminate all false-positives.

[zeroasterisk]

I am getting react/display-name warnings for the following anonymous stateless function syntax:

const options = {
  loadingHandler: () => (<LoadingSpinner />),
  errorHandler: (error) => (<ErrorBanner error={error} />),
  someOtherConfig: [1, 2, 3],
};

Aside from making variables for each, how could I even give this a name?

[ljharb]

By not using arrow functions.

const options = {
  loadingHandler() { return <LoadingSpinner />; }, // this
  errorHandler: function someOtherName() { return <ErrorBanner error={error} />; }, // or this
  someOtherConfig: [1, 2, 3],
}

[Kerumen]

@ljharb Really? This is clearly not a nice solution..

[ljharb]

@Kerumen i think it's much nicer. Named functions can be easier to read, because their names convey meaning - and arrow functions don't have explicit names. This is also incredibly important for debugging.

[aprilmintacpineda]

@zeroasterisk thanks for that, so anonymous functions are not valid as react components?

The error for me was because of this:

{
  //...
  component: props => (<MyJSX {...props} />),
  //...
}

I had to convert it to:

const aComponent = props => (<MyJSX {...props} />);

{
  //...
  component: aComponent,
  //...
}

which fixed the problem. It's weird though, because aComponent is a variable that is assigned an anonymous function... so.... I suppose it considers that as a named function? I don't know, can someone explain this?

[ljharb]

@aprilmintacpineda the function being anonymous or named is irrelevant; the only thing that matters is that the identifier name used in JSX to refer to the element starts with a capital letter.

Prior to React 16, any function that returned a node (null, or a React Element) was technically a component - now, a component can also return an array, a string, or a number.

[johnomalley]

This really really sucks when you have a file with a rendering function and a component.

import React from 'react'
import T from 'prop-types'

const renderItem = ({id, name}) =>
  <li key={id}>{name}</li>

const Example = ({items}) =>
  <ul>
    {items.map(renderItem)}
  </ul>

Example.propTypes = {
  items: T.arrayOf(T.object).isRequired
}

export default Example

The renderItem function is clearly not a component. It's not exported, nor is there any reference to <renderItem> in the file. But the component detection isnt smart enough to figure that out.

Even worse, there is apparently no way to suppress the inspection on renderItem without also turning off validation on the real component. Yuck.

I like the suggestion from @coderitual as a simple solution - as of now the prop-types and display-name inspections, as useful as they could be, are just too painful to enable in my project.

[ljharb]

renderItem should be a component - it should be an SFC, whether it's exported or not.

[coderitual]

I haven't seen a component which name starts lowercase. Even react is promoting this convention -> https://reactjs.org/docs/components-and-props.html

I agree it's not perfect solution but you cannot say whether expression is a SFC or normal function based on expresion itself. What makes the difference is how you call this expression.

All custom components in react must start upper case so it's unlikely to see something like this:

const comp = () => <div></div>;
const App = () => <comp></comp>;

Of course you can export lowercase function and change its name during import but still this is a bad practise.

I am not sure why only few people see this relation and simple solution to mitigate the problem.

Mike

[johnomalley]

renderItem should be a component

I completely disagree. Even if you think that any bit of JSX belongs in a formal component, which seems like a rather arbitrary and onerous requirement, at least a few libraries don't agree with that. For example, I use react virtualized, which employs rendering functions - called from the render methods of other components - to only render what is visible on the screen.

Also, extracting a function can greatly improve readability in the case where extracting a component wouldn't add any significant value.

edit - typo

[dimaqq]

Would the following example be an instance of this issue:

function Table(props) {
  return <ReactTable columns={[{ /* ... */
    accessor: (r) => r.x?"OK":<span className="bad">bad</span>}]}
  </ReactTable>
}

eslint complains that according to react/display-name, the span line is missing display name 😖

[ljharb]

yes, because “accessor” is a function that returns jsx.

[G-Rath]

I'm getting this too:

const renderWithClassName = (className: string) => (...props: Array<JSX.IntrinsicAttributes>) => <div className={className}{...props} />;

const ActualComponent = (props: Props) => {
   const classes = useStyles(props);

   const verticalTrackRenderer = useMemo(() => renderWithClassName(classes.trackVertical), [classes.trackVertical]);
}

What I'm interested in is just how much of an impact this'll have. The rule description says:

This name is used by React in debugging messages

but to me that doesn't well enough inform me of the impact that not having a displayName will have. I mean if it's just adding a little bit more detail to debugging messages, then that could be fine completely, provided its not the only piece of information provided for debugging.

On the other hand, if it means when an error happens the only thing I'm going to be told is "Component exploded!" without any other tie back to where the error happened, then ensuring every component has a displayName, including very small "middlemen" components like the one in my code above is very important.

I know that this can be hard to define, since eslint [plugin] isn't tied directly into React, and so who knows what new cool way of utilising the displayName people could come up with, but at least for me it'd be nice to have a bit more information on the impact of not having one 😃

[ljharb]

Debugging isn't "just"; it's critical. "when an error happens" is when you need debugging.

[donysukardi]

can we have the option to disable the rule when detecting lowercase functions? i'm using this with react-native's SectionList and getting warned for the renderItem and renderSection that are supposed to be just functions.

Assigning propTypes there would do nothing as the check won't be run against functions

[ThiefMaster]

I'd like to be able to exclude lowercase functions as well. Especially with JSX being widespread nowadays, a function name starting with a lowercase character is never a component.

[alexandermckay]

FYI. A similar problem exists when using higher order functions to generate components.

const entityRow = definition => entity => // eslint-disable-line react/display-name
  (
    <TableRow key={entity.id}>
      {definition.map(def => <TableRowColumn key={def}>{entity[def]}</TableRowColumn>)}
    </TableRow>
  )

If you'd rather not disable the rule when creating HOCs, wrapping TableRow with fragments will silence the error.

[ljharb]

(It shouldn’t; that’s a bug in the component detection)

[allexysleeps]

I think an option to disable lint errors for a lowercase function is the best way to handle it.
Because replicating the same props over and over again is kinda frustrating.

[dimaqq]

When functional components were just introduced, I recall writing a lot of “non-component functions that return jsx” mostly as helper functions to a component.

However, today, with hooks, I almost never do. Perhaps because components get simpler, or because individual hooks can often be refactored out into the helper functions which makes the latter components in their own right.

Although that’s not what OP was about, I feel that today the lint rule heuristic is much less of a nuisance. Overall the rule exists for a reason, so I too am happy annotate the OP as a rare special case.

[ljharb]

None the less, I'd be happy to review a PR that fixed the component detection to treat lowercased-named functions-that-return-jsx as "not components".

[tresdev]

How about not applying prop-types rule to functions inside components?

[BairDev]

By not using arrow functions.

const options = {
  loadingHandler() { return <LoadingSpinner />; }, // this
  errorHandler: function someOtherName() { return <ErrorBanner error={error} />; }, // or this
  someOtherConfig: [1, 2, 3],
}

Interestingly you cannot mix the way of function definition. E.g. using react-intl:

<FormattedMessage
    id="user:form:privacyAgreementText"
    values={{
        a: function a(textFrag: string) {
            return (
                <a href="myexample.com">{textFrag}</a>)
        },
        b: (textFrag: string) => (<b>{textFrag}</b>),
        p: (textFrag: string) => (<p>{textFrag}</p>),
        h3: (textFrag: string) => (<h3>{textFrag}</h3>),
        i: (textFrag: string) => (<i>{textFrag}</i>),
    }}
/>

Gives horrible complains by the TypeScript compiler.

[jp7837]

Is this potentially fixed by #2699 ?

[ljharb]

I believe so; happy to reopen if not.

[efe-imo]

By not using arrow functions.

const options = {
  loadingHandler() { return <LoadingSpinner />; }, // this
  errorHandler: function someOtherName() { return <ErrorBanner error={error} />; }, // or this
  someOtherConfig: [1, 2, 3],
}

Interestingly you cannot mix the way of function definition. E.g. using react-intl:

<FormattedMessage
    id="user:form:privacyAgreementText"
    values={{
        a: function a(textFrag: string) {
            return (
                <a href="myexample.com">{textFrag}</a>)
        },
        b: (textFrag: string) => (<b>{textFrag}</b>),
        p: (textFrag: string) => (<p>{textFrag}</p>),
        h3: (textFrag: string) => (<h3>{textFrag}</h3>),
        i: (textFrag: string) => (<i>{textFrag}</i>),
    }}
/>

Gives horrible complains by the TypeScript compiler.

Hey @BairDev , were you able to resolve this?

[BairDev]

Hey @BairDev , were you able to resolve this?

Hey, not really, I just use the same pattern for the mini-components.