False positive display-name when using forwardRef

[Sharcoux]

Tell us about your environment

• ESLint Version: 5.16.0
• Node Version: 10.12.0
• npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using? babel-eslint 10.0.1

Please show your full configuration:

Configuration

module.exports = {
    "env": {
        "browser": true,
        "es6": true
    },
    "extends": [
        "eslint:recommended",
        "plugin:react/recommended"
    ],
    "parserOptions": {
        "ecmaVersion": 9,
        "sourceType": "module",
        "ecmaFeatures": {
            "jsx": true
        }
    },
    "rules": {
        "indent": [
            "error",
            2,
            { "SwitchCase": 1 }
        ],
        "linebreak-style": [
            "error",
            "unix"
        ],
        "no-console": "warn",
        "quotes": [
            "error",
            "single"
        ],
        "semi": [
            "error",
            "always"
        ]
    }
};

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

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

const Paragraph = React.forwardRef(function Paragraph({ style }, ref) {
  // const finalStyle = Object.assign({}, style);
  return <div ref={ref} style={style} />;
});

Paragraph.propTypes = {
  style: PropTypes.object
};

export default Paragraph;
export const proptype = Paragraph.PropTypes;

npx eslint <file>

What did you expect to happen?
Nothing as the file is valid

What actually happened? Please include the actual, raw output from ESLint.

  4:19  error  Component definition is missing display name  react/display-name

✖ 1 problem (1 error, 0 warnings)

Fun fact! If you uncomment the line with Object.assign, the false positive disappears...

[ljharb]

hmm, the inner function is not in fact a component, because it takes ref as the second argument - and the forwardRef would need an explicit .displayName added to it.

[JustFly1984]

I have pretty the same issue with React.memo, and as it was explained, I did the same for forwardRef - and error disappeared.

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

function Paragraph({ style }, ref) {
  return (<div ref={ref} style={style} />)
}

Paragraph.propTypes = {
  style: PropTypes.object
};

export default React.forwardRef(Paragraph)

[Sharcoux]

Oh, ok. In same ways it makes sense.

Any idea about why the Object.assign stops the detection?

[ljharb]

A forwardRef callback is not a component and shouldn’t have propTypes; a component doesn’t get a ref argument.

[Sharcoux]

Well, then... What is the expected way to deal with it? Could you provide a code sample?

[ljharb]

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

const Paragraph = React.forwardRef(({ style }, ref) => {
  const finalStyle = Object.assign({}, style);
  return <div ref={ref} style={finalStyle} />;
});

Paragraph.displayName = 'Paragraph';
Paragraph.propTypes = {
  style: PropTypes.object
};

export default Paragraph;
export const proptype = Paragraph.PropTypes;

[Hypnosphi]

From rule motivation:

This name is used by React in debugging messages

React uses getComponentName function in those messages. In case of forwardRef, it derives the name from type.render which is the function passed to forwardRef. Third party tools like Enzyme use a similar logic.

So the fact that this function is not a component doesn't mean that its name (or even .displayName) has no effect on the actual displayed name

[ljharb]

Fair, but this rule is about component display names.

[Hypnosphi]

In the provided case, the Paragraph component (the return value of forwardRef) gets a display name of "ForwardRef(Paragraph)" recognized by both React and Enzyme

[ljharb]

It gets one regardless of the name of the callback function tho, right?

[Hypnosphi]

If the callback function has no .name or .displayName, the display name becomes just "ForwardRef" which is far less usefull. See https://github.com/facebook/react/blob/42b75ab007a5e7c159933cfdbf2b6845d89fc7f2/packages/shared/getComponentName.js#L36

Here, innerType is the callback function

[davidwieler]

I completely agree with @Hypnosphi, without a .name or .displayName debugging can be a nightmare.

Simple example:

Before upgrading to Unicorn, I just had:

import React, { forwardRef } from 'react'

const Button = forwardRef(({ className, active, ...props }, ref) => (
    <span {...props} ref={ref} className={`${className} ${active ? 'active' : ''}`} />
))

Was erroring out (for a reason not shown above), and was just showing me forwardRef in the error handler and it took me a bit to find where the issue was. Simply adding Button.displayName = 'SlateToolbarButton' would have saved me some time.

const Button = forwardRef(({ className, active, ...props }, ref) => (
    <span {...props} ref={ref} className={`${className} ${active ? 'active' : ''}`} />
))

Button.displayName = 'SlateToolbarButton'

[Hypnosphi]

@davidwieler the question is, whether the following work as well. I say that the following is just as good it terms of debugging, note the named function:

const Button = forwardRef(function StateToolbarButton({ className, active, ...props }, ref) {
    return <span {...props} ref={ref} className={`${className} ${active ? 'active' : ''}`} />
})

Can you please check it in your setup if you have any doubts?

[hon2a]

Regardless of how it looks when debugging, this is a false positive. It asks for the function passed to forwardRef to be given a displayName, to which (if I use Object.assign(fn, { displayName })) the latest React responds with a warning explicitly stating that the function passed to forwardRef is not supposed to be a component.

This is not a problem of just this rule, but a conceptual problem of how these rules detect React components. I'm getting similar false positives for both react/display-name and react/prop-types for other render functions (e.g. passed to antd's Table component to render cell values). Functions that accept multiple arguments simply shouldn't be considered React components, even if they produce React elements.

[ljharb]

@hon2a react function components accept 2 arguments already, props and context.

[ljharb]

However, certainly if React gives a runtime warning about having a displayName in certain cases, this rule shouldn’t be suggesting it then. Would you mind opening up a PR, with test cases?

[hon2a]

@hon2a react function components accept 2 arguments already, props and context.

@ljharb Could you, please, point me to any documentation stating that? Aren't you confusing it with constructor contract for class components?

However, certainly if React gives a runtime warning about having a displayName in certain cases, this rule shouldn’t be suggesting it then. Would you mind opening up a PR, with test cases?

I can look into creating simple test cases. If by PR you mean with a fix as well, that I can't promise, at least not at this time :)

[ljharb]

It’s how SFCs have worked since their inception, in React 0.14. I can dig up documentation later; but i have production code in use for years that relies on it.

Test cases would be great :-)

[hon2a]

I guess it's been too long since I used legacy context. Funny thing is, I can't find a mention of that anywhere.

[ljharb]

It’s also possible it’s been undocumented since new context came out ¯\_(ツ)_/¯ it still works tho

[jedwards1211]

This is supposed to be fixed right? I'm on 7.16.0 and still getting a false positive here:

/**
 * @flow
 * @prettier
 */

import * as React from 'react'
import classNames from 'classnames'

export type Props = {
  +className: string,
  +children: React.Element<any>,
}

const CloneWithClassName = React.forwardRef(
  ({ className, children }: Props, ref: React.ElementRef<any>): React.Node => {
    const child = React.Children.only(children)
    return React.cloneElement(child, {
      ref,
      className: classNames(child.props.className, className),
    })
  }
)

export default CloneWithClassName

[ljharb]

@jedwards1211 it's fixed in master but unreleased.

[jedwards1211]

okay thanks, I guess there are other false positive fixes in the changelog

[ljharb]

@jedwards1211 https://github.com/yannickcr/eslint-plugin-react/blob/master/CHANGELOG.md is accurate; the git log certainly might contain unreleased stuff in any repo :-)

[jedwards1211]

Heh, well since when I upgraded I got 7.20.0, and that changelog only goes to 7.16.0...I'm not sure what to think
Sorry I must be getting mixed up with another package, I do have 7.16.0. But that at least tells me I have all the fixes in the changelog