Add react/no-redundant-should-component-update #1084
Conversation
jomasti
force-pushed
the
no-useless-scu
branch
from
88a161f to
619110d
Compare
docs/rules/no-useless-scu.md
Outdated
| ```jsx | ||
| class Foo extends React.PureComponent { | ||
| shouldComponentUpdate() { | ||
| return true; |
There was a problem hiding this comment.
is the "return true" important? if not, i'd maybe put a comment here or something to indicate "any code can go here" (same throughout)
There was a problem hiding this comment.
Yeah, I was thinking this could come up when writing it. I'll update.
lib/rules/no-useless-scu.js
Outdated
| return tokens[1] && tokens[1].type === 'Identifier' ? tokens[1].value : tokens[0].value; | ||
| } | ||
|
|
||
| return node.key.name; |
There was a problem hiding this comment.
does node always have a key property?
There was a problem hiding this comment.
From what I can see, yes. It's only ever checking properties from the class. Unless there's some other possible node that I'm not aware of for a class.
There was a problem hiding this comment.
Might there be one in the future? It would probably be good to be defensible here.
lib/rules/no-useless-scu.js
Outdated
| if (utils.isPureComponent(node)) { | ||
| var hasScu = hasShouldComponentUpdate(node); | ||
| if (hasScu) { | ||
| var className = node.id.name; |
There was a problem hiding this comment.
does node always have an id property?
|
I think "redundant" might be more specific than "useless". What do folks think? Also, I'm not sure if we should abbreviate shouldComponentUpdate as scu. I think it might be best if the rule was named |
|
I agree with @lencioni's naming comments. |
|
I've updated the code based on the feedback given. I'll squash at the end after everything checks out. |
ljharb
changed the title
This will check for `shouldComponentUpdate` defined in a class that extends React.PureComponent.
|
@jomasti what do you think about my v14/v13/v15.0/v15.1 question? |
|
@ljharb It makes sense. Would you expect a test with settings for a lower version? All I can think of would be using the syntax with a lower version and having it not catch the error. It doesn't seem useful, though. |
|
@jomasti hm, that's a fair point - considering it'd just "do nothing" in a React where PureComponent also does nothing, it's probably not worth doing. |
|
I'll merge this tomorrow if there's no objections. |
This will check for
shouldComponentUpdatedefined in a class that extends React.PureComponent.In regards to the name: I went with what is in the proposal, but perhaps something more informative can be found. Also, I realized that the functions dealing with component properties are duplicated in a few spots, but since there were some differences, I figured they could be refactored out at some other point. That's ultimately up to the reviewers, though.
This PR implements the rule proposed in #985.