Add no-unused-state #1103
Add no-unused-state #1103
Conversation
|
I'm happy to port down to node 0.10 to get this merged sooner, but would rather #1038 be merged :) |
|
Yes, please do that. It will take much longer if it needs to wait for a major release, and this PR's diff will be cleaner. |
wbinnssmith
force-pushed
the
wbinnssmith/no-unused-state
branch
2 times, most recently
from
7ba0eaa to
4ddb0ff
Compare
|
Rebased and ported back to node 0.10 How do you feel about the lack of support for |
To clarify, it actually supports ANY class that has a |
There was a problem hiding this comment.
Since createClass still works and is supported by this plugin, I think it also needs to be handled.
As for the other point - many things have a render method that aren't React components. Please use the component detection utility functions that all of our rules use, and you should be able to cover all kinds of components without extra work.
|
Addressed the feedback and implemented createClass support. Components util made quick work of it -- awesome. |
lib/rules/no-unused-state.js
Outdated
| var isDirectStateReference = | ||
| node.type === 'MemberExpression' && | ||
| isThisExpression(node.object) && | ||
| node.property.name === 'state'; |
There was a problem hiding this comment.
will every node have a property property? If not, the .name could crash when encountering unexpected node types.
There was a problem hiding this comment.
every MemberExpression should, yeah, which should be limited by the prior condition. cc @rjbailey
There was a problem hiding this comment.
Yep, every MemberExpression should have a property AFAIK. It may not have a name (if it's computed), but it won't crash.
If we used getName(node.property) === 'state' instead we'd also handle the computed case (so we'd detect uses like this['state']). Looks like I forgot to do that here :)
|
@yannickcr @lencioni @EvNaverniouk ping for review |
|
Any update on this rule? |
|
@wbinnssmith would you mind freshly rebasing this? after that, i'll give it a rereview, and then merge ASAP if there's no objections from other collabs. |
|
@ljharb no problem :) |
wbinnssmith
force-pushed
the
wbinnssmith/no-unused-state
branch
from
17f9566 to
38bd88c
Compare
|
okay @ljharb, just rebased, cleaned things up, and moved some stuff back to node 4 compatible features (where this all started 😂) |
tests/lib/rules/no-unused-state.js
Outdated
|
|
||
| const eslintTester = new RuleTester({ | ||
| parserOptions: parserOptions, | ||
| parser: 'babel-eslint' |
There was a problem hiding this comment.
is babel-eslint needed for all these test cases? It'd be ideal to use the default parser whenever possible.
|
I can split the tests up using multiple RuleTesters, separating those that parse js compatible with espree + es2015, and those that require babylon with support for experimental features enabled (property initializers and object spread, mainly). Does that sound useful? |
wbinnssmith
force-pushed
the
wbinnssmith/no-unused-state
branch
from
ab4776a to
4bd7e37
Compare
|
You don't need to create a new RuleTester instance - the constructor argument is just the default parser options; each valid/invalid example can also provide its own parser options. |
|
@ljharb that's exactly what I ended up doing :) ping @yannickcr @lencioni @EvHaus |
|
ping! can @yannickcr @lencioni or @EvHaus look at this please? |
|
Sorry for the inconvenience; I've just merged a refactor to start using arrow functions. Would you mind rebasing? I'll merge within the next day or two, if no objections, after that's done. |
This adds a new rule, react/no-unused-state, which discovers state fields in a React component and warns if any of them are never read. It was developed internally at Facebook by @rjbailey and has been in use throughout Facebook's JS for a couple months now. It was written against a modern version of node, so has been rebased on @jlharb's branch dropping support for node < 4. It currently supports es2015 classes extending `React.Component` (no support currently for `React.createClass()`) and can detect when state is as `this.state = {}`, assigning state in a property initializer, and when calling `this.setState()`.
Link to significant commit is here
This adds a new rule, react/no-unused-state, which discovers state fields in a React component and warns if any of them are never read. It was developed internally at Facebook by @rjbailey and has been in use throughout Facebook's JS for a couple months now. It was written against a modern version of node, so has been rebased on @ljharb's branch dropping support for node < 4.
It currently supports es2015 classes extending
React.Component(no support currently forReact.createClass()) and can detect when state is asthis.state = {}, assigning state in a property initializer, and when callingthis.setState().