Added support of shouldComponentUpdate

[medyo]

I've added support of shouldComponentUpdate so you can control whatever a change should re-render the view or not.
For more details : Updating-shouldcomponentupdate
example :

var UserViewComponent = React.createBackboneClass({
    shouldComponentUpdate: function(nextProps, nextState){
        return false; // or any magic condition :)
    }
    render: function() {
        return (
          <div>
              <h1>{this.getModel().get("name")}</h1>
          </div>
        );
    }
});

[markijbema]

I don't think this does stuff in the right order. It only subscribes at moments the component should update, but it updates at any time, regardless of the shouldComponentUpdate. If you want to fix the shouldComponentUpdate you should make triggerUpdate compatible, the rest needs no changes.

[medyo]

I Caught subscribe action since it's the main responsible for calling the update action.
When you said make triggerUpdate compatible do you mean, kill the event at modelOrCollection.on(changeOptions, triggerUpdate, component) or handle the var triggerUpdate to be only responsive to shouldComponentUpdate

[clayallsopp]

I'm a little unsure about the need for this - maybe @markijbema has a stronger opinion.

If we are going to merge this in:

• Can we rename to shouldComponentRenderOnModelChanges, or something more explicit?
• Can you change README.md?
• Can you add a test?

[markijbema]

To be clear, I think this implementation is faulty. For instance, lets say you have an optimization that makes the component not update in the first few seconds of the lifecycle. A proper implementation would prevent it rendering in the first few seconds, this implementation will prevent it from rendering at all.

Again, I think it's a very good idea to support this method, but it should be checked at the proper moment in the lifecycle.

[markijbema]

I mean adding it to this condition:
https://github.com/medyo/react.backbone/blob/master/react.backbone.js#L36

[medyo]

Thanks guys for reviewing that, i've made the necessary edits.
Please check it, thanks

btw, the old tests were broken, i had to fix them before starting testing my code.

[markijbema]

is the undefined test really necesarry? I would expect react to make a default one if it isn't defined. Did you check this?

[tsjoberg]

added line note below on the same lines to call shouldComponentUpdate() directly in the triggerUpdate

[medyo]

the undefined is not really necessary, I am accustomed to check both cases undefined and null

[markijbema]

This changes two things: it adds the should-component update, and it changes the compatibility with react version. The createFactory is 'new' in react, so this won't work in older versions. Is there someplace in the readme where is mentioned which versions this library is compatible with? (I think it's a good idea to upgrade, but I think it should be mentioned somewhere, and it's probably good if @clayallsopp releases a new version after merging).

[tsjoberg]

Sorry I am also a little confused on the need as well - especially in the example (change:name). Wouldn't you just want to pass change:name to changeOptions which would trigger the forceUpdate() instead of checking it in shouldComponentUpdate() ?

[markijbema]

That's an example, and it can depend, for instance, let's say you have a UserComponent, which shows first and last name, and if expanded in the state is true, it also shows address. You can easily express this with shouldcomponent update, depending on state, but it's hard to express when you have to do this when binding the model.

So it is useful, but not totally expressed by the example. I think however that expanding the example will make it rather contrived. This is a standard React method though, and imo it would be good to support. Maybe we could link to the react docs from the readme?

[tsjoberg]

Couldn't you just add component.shouldComponentUpdate() here instead of calling helper function?

[medyo]

shouldComponentUpdate isn't always defined, for that reason i've added updatable function to handle condition there.

[tsjoberg]

Ok yes makes sense - strange that React doesn't have a default implementation for shouldComponentUpdate{return true;} and having the spec override the implementation (like other lifecycle methods). But looks like most implementations on React are like this and not Backbone esq where you override default methods with your own etc.

[tsjoberg]

@markijbema makes perfect sense thanks! - more so just making sure any shouldComponentUpdate() is not bypassed by the forceUpdate() called by the plugin. Think example just threw me off a bit

[clayallsopp]

It seems very weird to me that we're nominally adding support for something in vanilla React (shouldComponentUpdate) but not implementing it the same manner (i.e. passing nextProps and nextState). I image it would be confusing to users that we would hijack the definition; the reason that vanilla shouldComponentUpdate doesn't work is because of an implementation detail of this library (using forceRender)

I'd much rather make this difference explicit by choosing a different method name - i.e. shouldComponentUpdateOnModalChange

[markijbema]

Good point, I missed that, there's no reason not to pass the state and props though. Why not just pass them and let the user judge whter he want to depend on state, model or props changes to allow a render? I would not be jn favor of an alternaticpve method. Say I have a ckmpi ent which freezes after some time (for instance, only changes are allowed for two seconds, or when on the screen). If you make a seperate method you have to duplicate the logic of whether to display or not.

[clayallsopp]

Hm, so if we keep the same name, how does this implementation of shouldComponentUpdate interact with existing implementations?

I.e. I have a component and already have some logic in shouldComponentUpdate (such as if I store more properties in this.state) - are we okay with the side effect of this change being that shouldComponentUpdate gets called more often, with identical nextProps and nextState, and implicitly that this.getModel() has changed?

I'd like to see a test for that case, if we're okay with it

[medyo]

I think, it should be implemented otherwise at least like React did that.
I noticed that the first checking might return some undefined methods like this.state.count since it get called before initializing the component. i'll try to find a better way for that.

If you wanna do a test:

shouldComponentUpdate: function(nextProps, nextState){
    console.log("Props",nextProps, "State", nextState);
},

[markijbema]

Yes, it should be totally ok to call it more often.