Skip to content
This repository has been archived by the owner on Nov 10, 2017. It is now read-only.

Better state change checks & handling. #5

Closed
wants to merge 1 commit into from

Conversation

n1k0
Copy link
Owner

@n1k0 n1k0 commented Feb 3, 2015

Totally unsure about this one. This mostly wants to improve performances when state is updated often with the very same state values, to avoid unnecessary subscriber notifications.

I'm using JSON.stringify(state) comparison, because I don't want to go the JavaScript typechecks warpath, though that's probably limiting. That was a very bad idea. Back to standard strict check and basta. Someday we'll have builtin immutable structures, so we'll be able to safely compare things.

Needs feedback/review from @lionelB and/or anybody interested.

@n1k0 n1k0 force-pushed the better-state-change-handling branch from 746442b to c1ead11 Compare February 3, 2015 18:14
@n1k0 n1k0 force-pushed the better-state-change-handling branch from c1ead11 to fc2fa97 Compare February 3, 2015 22:54
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.16%) to 98.57% when pulling fc2fa97 on better-state-change-handling into 4be2f82 on master.

@lionelB
Copy link
Collaborator

lionelB commented Feb 4, 2015

1 - do you encounter the problem ? if so provide benchmark about performance boost
2 - I think calling function handler and copy variable are lightweight than JSON.stringify,
3 - Let react handle update if needed

@n1k0
Copy link
Owner Author

n1k0 commented Feb 4, 2015

1 - do you encounter the problem ?

Yes, that's why I made the PR tbh.

if so provide benchmark about performance boost

I tried using matcha but it's painfully broken with async benchmarks, wasted some hours on this yesterday evening, and using the setImmediate trick mentioned in the comments only brought more problems, so I decided not to bother with this.

2 - I think calling function handler and copy variable are lightweight than JSON.stringify,

Yeah, and as you can see latest version of the patch don't rely on serialization anymore, just strict equality check. I think that's a fairly decent compromise.

3 - Let react handle update if needed

That means implementing shouldComponentUpdate everywhere by hand; I really think stores could take care of basic checks like basic state identity change checks, to avoid users having to write too much boilerplate.

@lionelB
Copy link
Collaborator

lionelB commented Feb 4, 2015

thanks for the answer (and apologize because I rereading my comment and it sounds a bit imperative )
By the _ let react handle update _ I thought that react was smart enough to not update if state is not really updated and so no cause performance issues

@n1k0
Copy link
Owner Author

n1k0 commented Feb 4, 2015

Landed along 0ed606a.

@n1k0 n1k0 closed this Feb 4, 2015
@n1k0 n1k0 deleted the better-state-change-handling branch February 4, 2015 10:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants