FluxMixin: sync with stores when props change

[acdlite]

Uses componentDidUpdate() lifecycle method and a shallow object comparison of current props with previous props to check if props have changed. If so, gets current state from stores.

This does not check for state changes, because that would result in an infinite update loop. Need to update docs to warn about using this.state inside a store getter.

Fixes #29

[acdlite]

@quazzie Do you want to try this out and see if it's working for you?

[quazzie]

Sure, but how do i build flummox ? :)
I got the pull request in my local git clone.
I'm on windows.

[acdlite]

Hmm, they build system is make. I'm pretty sure you can install that on Windows, with cygwin or something?

If/when you have make installed, run make fast-build the first time and make build each time after that.

Or just run npm run test which will build and test.

[acdlite]

@quazzie After some more testing I'm going to go ahead and merge this. Let me know if you continue to have any problems, of course.

[msimulcik]

Why componentDidUpdate() and not componentWillReceiveProps()? According to documentation (http://facebook.github.io/react/docs/component-specs.html), calling this.setState() within componentWillReceiveProps() function will not trigger an additional render.

[acdlite]

@msimulcik Huh, I believe there's a reason I did that but now I can't remember haha. Seems very silly now that I'm looking at it again.

[acdlite]

@msimulcik Nope, it's that way for a reason. Tried switching it and the test case I wrote failed. I still can't remember why, though... something about how the lifecycle methods work.

[gaearon]

AFAIK you really should be using componentWillReceiveProps for that.

[gaearon]

But yeah, for componentWillReceiveProps to work properly, you need to pass its nextProps (first parameter) to getStoreState. The way people usually solve that is by always passing either props or nextProps there:

      let Component = React.createClass({
        mixins: [FluxMixin({
          test: function(props, store) {
            return {
              foo: 'foo is ' + props.foo,
            };
          },
        })],

Making it first parameter would be a breaking change, but the benefit is that people don't use this.props by mistake and componentWillReceiveProps works nicely.

[acdlite]

Ah yes, I remember now. This will change in the next major version. I don't see this as a huge problem right now though because

1. It only affects fluxMixin (which I'd really love to deprecate sooner rather than later, anyway) and directly nested FluxComponents
2. Current version works okay, just isn't ideal because it requires an extra render. But again, it shouldn't often be the case that you're using a component's own props inside store getters. Just use FluxComponent with the owner's props.

Think I'll cut a new version with this change around the time 0.13 is released, along with the re-tooled array form of connectToStores().

@gaearon I am curious why you suggest props as the first parameter. My instinct would be (store, props), since most of the time the props argument isn't even needed.

[gaearon]

@gaearon I am curious why you suggest props as the first parameter. My instinct would be (store, props), since most of the time the props argument isn't even needed.

Maybe you're right. My instinct is doing so because otherwise it's too easy to use this.props by mistake and forget the props parameter. If it's the first parameter, it's much more noticeable.

[acdlite]

@gaearon If we pass props as a parameter to the state getter, I think we could also get away with not binding the getter to the store. Then this.props wouldn't work anyway. I like this because it's a more functional pattern.

Or... as an upgrade path we could bind a mock context so that this.props is actually the next props, but that's probably too clever by half. :D

I've come around to the view that binding is usually not a good idea. If you're using this as a parameter, you're doing it wrong.

[gaearon]

I'm totally pro not-binding.