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

Fix no-unused-prop-types setState updater #1507

Merged
merged 1 commit into from
Nov 13, 2017

Conversation

petersendidit
Copy link
Contributor

@petersendidit petersendidit commented Oct 29, 2017

Handle when props are used in the setState update callback

Fix #1506

'class MyComponent extends React.Component {',
' onFoo() {',
' this.setState((prevState, props) => {',
' props.doSomething();',
Copy link
Member

Choose a reason for hiding this comment

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

does this work independent of what "props" is called?

Copy link
Contributor Author

@petersendidit petersendidit Oct 30, 2017

Choose a reason for hiding this comment

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

Not yet, working a better version that will handle more cases. This code doesn't handle destructuring or anything like that.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's important that it handles that from the get-go.

@jseminck
Copy link
Contributor

Does this case work with the prop-types rule? If not: the code is really similar and it should be possible to apply the same logic there.

Handle when props are used in the setState update callback

Fix jsx-eslint#1506
@petersendidit petersendidit changed the title Fix no-unused-prop-types arrow callbacks Fix no-unused-prop-types setState updater Nov 5, 2017
@petersendidit
Copy link
Contributor Author

Should handle this case much better now. Added a test case for when the second argument to the setState updater is not named props and is not destructured. Let me know if there are other test cases that should be added.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems legit

@jseminck
Copy link
Contributor

jseminck commented Nov 6, 2017

What about the prop-types rule? Does it also need this functionality?

@ljharb
Copy link
Member

ljharb commented Nov 6, 2017

Yes, I think it does.

@ljharb ljharb merged commit 64cf2d7 into jsx-eslint:master Nov 13, 2017
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.

4 participants