(#2272) adds warning for setting state through this.state

[cirocosta]

As it was addressed by #2272 , it'd be nice if we could alert the dev when doing something which may lead to messy things (in this case, setting the state directly with this.state). In this PR i try to implement that feature by using ES5' Object.defineProperty. I'd like to know from you guys what do you think about this.

My approach is: keep an internal _state protected so that the user won't touch it without getting alerted when setting it directly (inside an if __DEV__). Getting from it is safe.

I first thought about doing this through ES6s Proxies, which is something that i though it'd do more sense (creating defensive objects), but AFAIK nowadays implementations are not efficient and it'd be an overhead (correct if i am wrong, please!).

What do you think? Thanks!

[sebmarkbage]

This won't work in IE8 because defineProperty doesn't work there. Also, it'll be slow in production code even though it's not necessary. So we need to hide this whole getter/setter behind __DEV__.

In general, this seems like a good idea but I'm not sure how this will work once we switch to ES6 classes and arbitrary third-party classes since we have less control there. Not sure how exactly we will update the state property neither.

[syranide]

@sebmarkbage Wouldn't Object.freeze work very well here? But perhaps it's a bad to freeze the props object as we do not require the use of immutable data...

[sebmarkbage]

We'll probably make props immutable. But this is on the class instance where people put all kind of extensions like this._timer = setTimeout(...) which would be more of a stretch.

[cirocosta]

@syranide @sebmarkbage thanks for your feedback! Didn't know about the impossibility of shimming this to IE8 (without relying on creating 'fake' dom elements and then using the approach). @syranide as i remember (not completly sure), Object.freeze fails silently on IE8.

Didn't know about the plans for ES6 classes (for anyone interested, see Components as ES6 Classes, The road to 1.0 and Proposal for porting React's Mixin APIs to a generic primitive). Seems like a great approach 👍

[sophiebits]

@sebmarkbage Do we want to do some version of this or will this not work with ES6 stuff?

[sebmarkbage]

I think that we'll want to move the authoritative state object into a separate internal store. E.g. _currentState. Then we can simply compare against inst.state to see if it changed and warn in that case.

[jimfb]

@cirocosta I know this is an old one, but would you be interested in updating this one so we can merge it in?

As per Sebastian, we should use a shadow state object (like _currentState) and then calculate if the user modified state, and emit a warning if modified.

Any interest in fixing it up and merging it?

[cirocosta]

Hi @jimfb , thanks getting back to these old PRs! I'll only be able to work on this in 3 weeks (programming assignments and final exams happening all together :( ). Then i could surely update it :D

What do you think?

[jimfb]

@cirocosta Sounds good to me! Thanks! Good luck with your exams!

[jimfb]

Ping @cirocosta - How'd your final exams go? Any chance you have some time to rebase and use a shadow state object as per above?

[jimfb]

Ping @cirocosta

[cirocosta]

Hi @jimfb ! I'll get into it this weekend. Thanks for pinging!

[jimfb]

Ping @cirocosta

[gaearon]

I’m closing as the PR has been requested to change, but has not been updating since. The issue is still tracked in #2272, and you are welcome to submit a new PR that incorporates the feedback you received in #2283 (comment). Alternatively, if you are busy, somebody else might take it from here.

If you start working on it again, please let us know both here and in #2272 so somebody else doesn’t start at the same time. Thank you for taking the time to contribute!

[iansu]

Hi, I'd like to attempt to implement this using the _currentState solution suggested by @sebmarkbage.