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.

Sign up for GitHub

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

Jump to bottom

Allow this.state assignment on constructor #1223

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Allow this.state assignment on constructor #1223

merged 1 commit into from
Jun 21, 2017

Conversation

burabure
Copy link
Contributor

see #832

ljharb
ljharb requested changes May 25, 2017
View reviewed changes
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.

Can you add more invalid examples, including assignment to this.state inside a callback in the constructor, as well as in other lifecycle methods?

docs/rules/no-direct-mutation-state.md Outdated
@@ -1,8 +1,10 @@
# Prevent direct mutation of this.state (react/no-direct-mutation-state)

NEVER mutate `this.state` directly, as calling `setState()` afterwards may replace
Don't mutate `this.state` directly, as calling `setState()` afterwards may replace
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this line needs to be changed; assignment isn't mutation.

lib/rules/no-direct-mutation-state.js Outdated

Object.keys(list).forEach((key) => {
if (isValid(list[key])) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Might be simpler to invert the if and put the report inside

docs/rules/no-direct-mutation-state.md Outdated
the mutation you made. Treat `this.state` as if it were immutable.

The only place that it's acceptable to assign this.state is on a ES6 class component constructor
Copy link
Member

Choose a reason for hiding this comment

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

s/on/in, and add a period at the end. Also lets backtick class

@burabure burabure force-pushed the no-direct-state-mutation-allow-constructor branch from bbb3313 to 886b7a0 Compare May 29, 2017 13:40
@burabure
Copy link
Contributor Author

@ljharb ready for review

ljharb
ljharb reviewed May 30, 2017
View reviewed changes
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.

Looks good!

Could we also include an invalid test case like:

constructor() {
  super();
  doSomethingAsync(() => {
    this.state = 'bad';
  });
}

lib/rules/no-direct-mutation-state.js Outdated
@@ -49,11 +49,18 @@ module.exports = {
// --------------------------------------------------------------------------
// Public
// --------------------------------------------------------------------------
var inConstructor = false;
Copy link
Member

Choose a reason for hiding this comment

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

let's use let here.

@burabure burabure force-pushed the no-direct-state-mutation-allow-constructor branch from 886b7a0 to 09a7f93 Compare May 30, 2017 15:43
@burabure
Copy link
Contributor Author

@ljharb ready for review!

@burabure burabure force-pushed the no-direct-state-mutation-allow-constructor branch from 09a7f93 to 36c7ade Compare May 30, 2017 15:46
@valentin-radulescu-hs
Copy link

👍

@valentin-radulescu-hs valentin-radulescu-hs mentioned this pull request Jun 21, 2017
Closed
@ljharb ljharb added the bug label Jun 21, 2017
@ljharb ljharb merged commit 61b65a0 into jsx-eslint:master Jun 21, 2017
@supra28
Copy link

supra28 commented Aug 5, 2017

Has this been released yet?

@ljharb
Copy link
Member

ljharb commented Aug 5, 2017

No. You can tell by clicking on the commit hash - 61b65a0 - and seeing that it's in master, but there's no version tags that it's in.

@supra28
Copy link

supra28 commented Aug 6, 2017

@ljharb Thanks

@jsg2021
Copy link
Contributor

jsg2021 commented Aug 10, 2017

I was under the impression this.state = {} was a valid state reset/replacement.

@ljharb
Copy link
Member

ljharb commented Aug 10, 2017

Nope; React would have no way to know you'd done that, so it wouldn't trigger a rerender. You should only change state with this.setState.

@supra28
Copy link

supra28 commented Sep 2, 2017

@ljharb I have updated the plugin to 7.3.0 and I still get this error :(

@ljharb
Copy link
Member

ljharb commented Sep 2, 2017

@supra28 i'm not sure what error you mean. can you file a new issue?

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

@EvHaus EvHaus EvHaus approved these changes

@ljharb ljharb ljharb approved these changes

@yannickcr yannickcr Awaiting requested review from yannickcr

@lencioni lencioni Awaiting requested review from lencioni

Assignees
No one assigned
Labels
bug
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@burabure @valentin-radulescu-hs @supra28 @ljharb @jsg2021 @EvHaus