-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
react/display-name gives false positives with component subclasses + argument spreading + es7 class properties #1200
Comments
(note, class properties aren't ES7, which came out in June of 2016, they're stage 2 - so they're not ES anything yet)
|
Thanks for the quick response! Seems like class properties are encouraged by the React team, since their Can you elaborate why a manually bound class method is more correct and performant? |
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 |
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.
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 I can take a stab at putting together a PR that fixes this edge-case. Do you think that would be worthwhile? |
This is an aside, but...
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:
Would you consider that a valid use-case for a subrender function? Thanks :) |
That's only true if you only tbh, I'd still say that 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). |
I dug into this a bit more out of curiosity, and it seems like that the https://github.com/eslint/eslint/blob/v3.17.1/lib/util/source-code.js#L258 We're currently on: This PR seems to have changed the 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! |
Interesting - do you see the same behavior when you omit the type notations? |
Ah, I missed that. The error goes away if the type is removed.
|
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. |
Yep, I'll investigate a bit more when I get some more time. Thanks again for your help! |
The code below shows the bug:
The error goes away when you either:
MyComponent
to directly extendReact.Component
subrender
to be a normal class method instead of an es7 class propertysubrender
The text was updated successfully, but these errors were encountered: