react/display-name gives false positives with component subclasses + argument spreading + es7 class properties

[dounan]

The code below shows the bug:

import React from 'react';

class CustomComponent extends React.Component {}

/** @extends React.Component */
export default class MyComponent extends CustomComponent {
  render() {
    return this.subrender();
  }

  // ERROR: component definition missing display name
  subrender = (...args: Array<*>) => {
    return <div />;
  };
}

The error goes away when you either:

• Change MyComponent to directly extend React.Component
• Change subrender to be a normal class method instead of an es7 class property
• Remove argument spreading in subrender

[ljharb]

(note, class properties aren't ES7, which came out in June of 2016, they're stage 2 - so they're not ES anything yet)

subrender is a function that returns JSX, so in general, that's a component - whereas a class method is not. Separately, both for correctness and performance, you want that to be a class method that's manually bound in the constructor.

[dounan]

Thanks for the quick response!

Seems like class properties are encouraged by the React team, since their class.js codemod generates them by default: https://github.com/reactjs/react-codemod/blob/master/transforms/class.js#L944

Can you elaborate why a manually bound class method is more correct and performant?

[ljharb]

That's unfortunate, but "encouraged by the React team" doesn't mean they're right :-)

Basically, instead of creating N functions, one for each instance, you'd instead have 1 prototype function for all instances, and the only part that would be created N times is the this.foo = this.foo.bind(this) - leaving the engine able to optimize the "meat" in the prototype method. This is a JS thing, not specific to react. Public class properties are appropriate for data; not for functions/methods.

[dounan]

Basically, instead of creating N functions, one for each instance

That makes sense. I can see how if you used class properties for all methods, it would be less performant. But manually binding in the constructor, then setting it as a property on the instance does the same thing, so in those cases there would be no performance benefit.

That's unfortunate, but "encouraged by the React team" doesn't mean they're right

Agreed, but I'm working with a pretty large codebase, and if the codemod has already added all these class properties, I don't know if it is worth the effort to encourage the engineering team to manually fix everything.

We have a need for the custom component class, so we can't get rid of that. But the argument spreading can be removed in most cases I think. The other option would be to disable the display-name lint rules in the symptomatic files, but that doesn't feel right.

I can take a stab at putting together a PR that fixes this edge-case. Do you think that would be worthwhile?

[dounan]

This is an aside, but...

subrender is a function that returns JSX, so in general, that's a component

I typically agree with this, but there are certain cases where the subrender function does a few conditional checks before rendering something simple. For example:

renderIcon() {
  if (this.props.item.done) {
    return <span className="check" />;
  } else if (this.props.item.urgent) {
    return <span className="urgent" />
  } else {
    return null;
  }
}

Would you consider that a valid use-case for a subrender function? Thanks :)

[ljharb]

But manually binding in the constructor, then setting it as a property on the instance does the same thing, so in those cases there would be no performance benefit.

That's only true if you only new the component once. If it's constructed multiple times, then you absolutely get a benefit when using the constructor-bound approach.

tbh, I'd still say that renderIcon should be an Icon SFC, no matter how simple it is.

Regardless, eslint-plugin-react has no way of knowing that a function that returns JSX isn't a component, unless it's a class method (which technically could be a component, but is highly unlikely to be).

[dounan]

I dug into this a bit more out of curiosity, and it seems like that the getJSDocComment was not returning the /** @extends React.Component */ comment when the argument spreading operator is there. This is because the leadingComments field of the ClassDeclaration node is not there when the argument spreading syntax is used.

https://github.com/eslint/eslint/blob/v3.17.1/lib/util/source-code.js#L258

We're currently on: eslint 3.17.1 babel-eslint 7.1.1

This PR seems to have changed the getJSDocComment behavior: https://github.com/eslint/eslint/pull/7516

I didn't dig into the PR, so I'm not sure if that will resolve this issue. Hopefully a later version of eslint or babel-eslint will fix this. Thanks again for your help @ljharb!

[ljharb]

Interesting - do you see the same behavior when you omit the type notations?

[dounan]

Ah, I missed that. The error goes away if the type is removed.

subrender = (...args) => { works

[dounan]

Not sure if this is related, but using ASTExplorer shows that the flow parser puts the comments in a different part of the AST (not as a leadingComments prop):

[ljharb]

In that case, it seems like it's a bug with the Flow parser, since it should probably not alter the way non-type-annotated nodes are parsed.

[dounan]

Yep, I'll investigate a bit more when I get some more time. Thanks again for your help!