oktavilla / backbone-kinship Goto Github PK

When cloning a kinship model it's relational properties does not get included... I'm guessing because (original backbone code):

clone: function() {
  return new this.constructor(this.attributes);
}

... the relational attributes are gone at this point.

ping @snurra

I tried unsetting a related property that was a collection. The collection was still there afterwards.

Doing it that way is just asking for trouble. Sooner or later someone will set up a relation with a name that clashes with an already existing Backbone.Model property or method. Plus it's ugly.

I suggest storing them in Backbone.RelationalModel._relations.

A consequence is that Backbone.RelationalModel.someRelation will stop working. However, there is already the alternative (or even preferable) way of Backbone.Model.get("someRelation").

Example:

var Company = Backbone.RelationalModel.extend({
  relations: {
    employees: Backbone.Collection.extend({})
  }
});
var aCompany = new Company();
// Listening to all changes in our company
aCompany.on("change", function() {
  console.log(arguments);
});
aCompany.set("name", "Kinship ltd");
aCompany.set("employees", [{ name: "Martin" }, { name: "Gustaf" }]);

Currently, the arguments outputted from the first change event are:

The aCompany Model
Set options

The arguments from the second and third change events are:

The employee model
The employees collection
Set options

My suggestion is that the arguments from the seconds and third change events should be:

The aCompany model
Set options

This change would keep this in line with how non-relational change events already work. It's also possible that aModel.changed should be updated with the changed employee models to reflect this change in a proper way.

Also, I suggest that delegated add and remove event should follow the same pattern and send the kinship model as first attribute and the changed collection (as JSON?) as the seconds attribute.

Normal change events are triggered after all changes needed from a call to Backbone.Model.set has been made. However, delegated change events are triggered after each change made to the related attribute.

Example:

var Company = Backbone.RelationalModel.extend({
  relations: {
    employees: Backbone.Collection.extend({})
  }
});
var aCompany = new Company();
// Set 1
aCompany.set("name", "Kinship ltd");
// Set 2
aCompany.set("employees", [{ name: "Martin" }, { name: "Gustaf" }]);
// Set 3
aCompany.set({ name: "Kinship.com", employees: [{ name: "Henric"}, { name: "Mateusz" }]});

With current functionality:
Set 1 triggers 1 change:name event (after all attributes have been changed).
Set 2 triggers 2 change:employees events (one after each added employee).
Set 3 triggers 4 change:employees events (one after each removed and added employee) and 1 change:name event (after all attributes have been changed).

My suggestion is this:
Set 2 triggers 1 change:employees event (after all attributes have been changed).
Set 3 triggers 1 change:employees event and 1 change:name event (after all attributes have been changed).

Also, make sure that Set 2 triggers a change event after all related attributes have changed. (Not sure if this is currently done or not.)

This will bring delegated changes more in line with how normal changes behave (one change event for each changed attribute). Also, the model will be in a completely changed state instead of partial when the change event is triggered, just as with a normal change event.

Related to #7

Currently a relation is only set up (ie an instance is created) when a value is set for that relation. Before that it's undefined.

This leaves instancing up to other places in code where you always have to check whether a relation is set up or not before trying to modify it, to avoid errors. I think it would be leaner if you could rely on all relations being set up, with or without values.

@martinlissmyr Any thoughts on why/where this might be bad idea?