Add option to use a callback to retrieve a model or collection from the component’s props

[s0meone]

This feature allows you to specify a callback to retrieve the Backbone model or collection to listen to.

Some of our models have a model defined as a property. This method allows you to listen for changes on those models as well.

Example:

var CommentView = React.createBackboneClass({
    mixins: [
        React.BackboneMixin("comment"),
        React.BackboneMixin(function(props) {
            return props.comment.user;
        })
    ],
    render: function() {
        return (
          <div>
              <p>{this.props.comment.get("text")}</p>
              <p>{'posted by' + this.props.comment.user.get("name")}</h1>
          </div>
        );
    }
});

[clayallsopp]

Ah cool, thanks for submitting! I don't have a strong opinion on whether or not this feature should be included, would like to hear @markijbema's or any other opinions

I think my only strong opinion is that the API is a bit non-intuitive atm - something that makes the purpose of the callback more clear would be preferable, maybe like:

React.BackboneMixin({
  renderOnChangesTo: function(props) {
     return props.comment.user;
  }
});

(naming things is hard)

[EtienneLem]

Very nice indeed. I think a named attribute for the callback is the way to go, it is more future proof if you ever want to add other optional attributes.

I would also expect renderOnChangesTo to support array:

React.BackboneMixin({
  renderOnChangesTo: function(props) {
    var comment = props.comment;
    return [
      comment,
      comment.user
    ];
  }
});

[s0meone]

Thanks for the feedback, it makes sense. I'll change the argument to an object with a named attribute.

Being able to return an array is a nice addition as well. But do you have any ideas what to do with the second argument customChangeOptions? We could apply the same options for every model returned from the callback, but I'm not sure if that feels intuitive enough.

This is a bigger change than I anticipated, but I can make some time this week to finish this.

[clayallsopp]

At first glance, I feel like customChangeOptions should be ignored if you're passing in renderOnChangesTo. That does muddle the API a bit, so not set on it, but there could exist a supported syntax like:

React.BackboneMixin({
  renderOnChangesTo: function(props) {
    var comment = props.comment;
    var options = {};
    options[comment] = ["change:title", "change:date"];
    options[comment.user] = ["change:name"];
    return options;
  }
});

[s0meone]

I feel like adding the option to return an array, complicates the API too much. You can always create multiple mixins. How about something like this:

React.BackboneMixin({
  modelOrCollection: function(props) {
    return props.comment.user;
  },
  listenTo: "change:name"
});

And:

React.BackboneMixin("comments", "add");

is equivalent to:

React.BackboneMixin({
  propName: "comments",
  listenTo: "add"
});

Adding the propName attribute makes this easier to implement and stay backwards compatible.

[clayallsopp]

Ah yeah, definitely agree that using multiple mixins is preferable to making the API more complicated

I'm good to go with your last API suggestions - would you be open to changing the name of the listenTo option? I think a label that has render or update in there would be more helpful, which more clearly states what happens when those events fire (renderOn, renderEvents, updateEvents, etc)

[s0meone]

Good idea, renderOn seems like a better fit. I'll update this PR as soon as possible.

[s0meone]

I've replaced the commit with a new one. This is now all possible:

You can pass an object with options to the included mixin:

React.BackboneMixin({
    propName: "user",
    renderOn: "change:name"
});

And supply a custom callback to the option modelOrCollection to retrieve the property from the component's props:

React.BackboneMixin({
    modelOrCollection: function(props) {
        return props.comment.user;
    },
    renderOn: "change:name"
});

And this change is backwards compatible, so this still works:

React.BackboneMixin("comments", "add");

[clayallsopp]

Awesome, and the diff is pretty clean :)

Feels like we should start adding tests - perhaps Jest eventually

[s0meone]

Thank you :)

On Wed, Jun 18, 2014 at 9:44 PM, Clay Allsopp notifications@github.com wrote:

Awesome, and the diff is pretty clean :)

Feels like we should start adding tests - perhaps Jest eventually

Reply to this email directly or view it on GitHub: https://github.com/usepropeller/react.backbone/pull/23#issuecomment-46484335

[markijbema]

I really like this, and intended to create something like it. However, the current version is to simple. It assumes that the function is a pure one, and exactly for the case which you describe, this probably isn't true.

So lets say you have a user, and a user has a property last_comment. Now the user creates a new comment, and last_comments becomes a pointer to the new comment. Now the function will never return the reference to the previous last_comment, and will never be able to unbind.

This means that this implementation will create memory/event leaks whenever any part of this function is not pure (and this is highly likely with backbone). This is exactly the reason I didn't implement this yet, because it's a bit of work to do it correctly.

The correct method to do this would be to save a reference to the object at the moment it's bound, and on propschange and events like that, you need to check against the cached version.

So my suggestion would be to either properly fix this, or otherwise add a very explicit warning that this behaviour is very unsafe when used with unpure functions.

(apologies for the slow reaction, I was on holiday, so only checked my notifications just now)