Skip to content
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

Add no-unused-state #1103

Merged
merged 6 commits into from Jul 21, 2017
Merged

Conversation

wbinnssmith
Copy link
Contributor

@wbinnssmith wbinnssmith commented Mar 7, 2017

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 for React.createClass()) and can detect when state is as this.state = {}, assigning state in a property initializer, and when calling this.setState().

@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Mar 7, 2017

I'm happy to port down to node 0.10 to get this merged sooner, but would rather #1038 be merged :)

@ljharb
Copy link
Member

ljharb commented Mar 7, 2017

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 wbinnssmith force-pushed the wbinnssmith/no-unused-state branch 2 times, most recently from 7ba0eaa to 4ddb0ff Compare Mar 10, 2017
@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Mar 10, 2017

Rebased and ported back to node 0.10

How do you feel about the lack of support for React.createClass()?

@ianobermiller
Copy link

ianobermiller commented Mar 10, 2017

It currently supports es2015 classes extending React.Component

To clarify, it actually supports ANY class that has a render method, no matter what it extends from.

ljharb
ljharb previously requested changes Mar 11, 2017
Copy link
Member

@ljharb ljharb left a 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.

@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Mar 13, 2017

Addressed the feedback and implemented createClass support. Components util made quick work of it -- awesome.

Copy link
Member

@ljharb ljharb left a comment

Nice! This LGTM but I'm going to defer to other reviewers for the final stamp :-)

var isDirectStateReference =
node.type === 'MemberExpression' &&
isThisExpression(node.object) &&
node.property.name === 'state';
Copy link
Member

@ljharb ljharb Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will every node have a property property? If not, the .name could crash when encountering unexpected node types.

Copy link
Contributor Author

@wbinnssmith wbinnssmith Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every MemberExpression should, yeah, which should be limited by the prior condition. cc @rjbailey

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@ljharb ljharb dismissed their stale review Mar 13, 2017

deferring to other reviewers

@ljharb ljharb requested review from yannickcr, lencioni and EvHaus Mar 13, 2017
@ljharb
Copy link
Member

ljharb commented Apr 22, 2017

@yannickcr @lencioni @EvNaverniouk ping for review

@danielheyman
Copy link

danielheyman commented Jul 3, 2017

Any update on this rule?

@ljharb
Copy link
Member

ljharb commented Jul 3, 2017

@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.

@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Jul 7, 2017

@ljharb no problem :)

@wbinnssmith wbinnssmith force-pushed the wbinnssmith/no-unused-state branch from 17f9566 to 38bd88c Compare Jul 7, 2017
@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Jul 7, 2017

okay @ljharb, just rebased, cleaned things up, and moved some stuff back to node 4 compatible features (where this all started 😂)

ljharb
ljharb approved these changes Jul 11, 2017
Copy link
Member

@ljharb ljharb left a comment

LGTM


const eslintTester = new RuleTester({
parserOptions: parserOptions,
parser: 'babel-eslint'
Copy link
Member

@ljharb ljharb Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is babel-eslint needed for all these test cases? It'd be ideal to use the default parser whenever possible.

@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Jul 11, 2017

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 wbinnssmith force-pushed the wbinnssmith/no-unused-state branch from ab4776a to 4bd7e37 Compare Jul 12, 2017
@ljharb
Copy link
Member

ljharb commented Jul 12, 2017

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.

@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Jul 13, 2017

@ljharb that's exactly what I ended up doing :)

ping @yannickcr @lencioni @EvHaus

@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Jul 20, 2017

ping! can @yannickcr @lencioni or @EvHaus look at this please?

@ljharb
Copy link
Member

ljharb commented Jul 20, 2017

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.

EvHaus
EvHaus approved these changes Jul 21, 2017
Copy link
Collaborator

@EvHaus EvHaus left a comment

LGTM

wbinnssmith added 2 commits Jul 21, 2017
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()`.
@wbinnssmith wbinnssmith force-pushed the wbinnssmith/no-unused-state branch from 4bd7e37 to ba1dfc1 Compare Jul 21, 2017
@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Jul 21, 2017

done.

@ljharb ljharb merged commit 8bfafe1 into jsx-eslint:master Jul 21, 2017
3 checks passed
@wbinnssmith
Copy link
Contributor Author

wbinnssmith commented Jul 22, 2017

🎉 Thanks for your help throughout the process, @ljharb!

@ljharb
Copy link
Member

ljharb commented Aug 10, 2017

Whoops, this needs documentation. @wbinnssmith, if you could file a PR to fix #1351, that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

6 participants