dhruvaray / backbone-associations

Create object hierarchies with Backbone models; Respond to hierarchy changes using regular Backbone events.
http://dhruvaray.github.io/backbone-associations/
MIT License
492 stars 75 forks source link

Updating to 0.6.2: _proxyCallback issues #145

Closed chiplay closed 10 years ago

chiplay commented 10 years ago

I'm trying to get our app up to date with the latest changes to 0.6.2 but I'm running into one issue. The if(!relationValue._proxyCallback) { check that happens on line 355 is preventing some of my nested models from bubbling correctly to their parents. The issue arrises when there is already a relationship established with one parent, then a second parent also wishes to relate to to the same model - and relationValue._proxyCallback already exist. In my use case the two parent models have different relationKeys - so the _proxyCallback function only bubbles correctly for the first callback that was registered.

Any thoughts on how to allow a child model to bubble events to two different parents, with two different relationKeys? Thanks!

chiplay commented 10 years ago

Here's an attempt at some sudo code for the issue:

var Foo = Backbone.AssociatedModel.extend({});

var Bar = Backbone.AssociatedModel.extend({
       relations: [{
       type: Backbone.One,
            key: 'foo_a',
            relatedModel: Foo
       }]
});

var Baz = Backbone.AssociatedModel.extend({
       relations: [{
       type: Backbone.One,
            key: 'foo_b',
            relatedModel: Foo
       }]
});

Create some instances:

var foo = new Foo();

var bar = new Bar({ foo_a: foo});
var baz = new Baz({ foo_b: foo});

And some listeners:

bar.on('change:foo_a.name', someMethod); => fires correctly
baz.on('change:foo_b.name', someMethod); => does not fire

Now when we have an event - foo.set({ name: 'John' }); - only bar bubbles the event from foo because the _proxyCallback method on foo has the relationKey foo_a in its closure.

All that said - I'm not sure I have a solution for registering mulitple _proxyCallbacks for each model, perhaps its an array or object with the relationKey as the unique identifier?

chiplay commented 10 years ago

Hey @dhruvaray - I think I've got a working solution - I'm a bit crunched for time this week, but let me know if you need a proper PR for this:

Line 351:

relationValue._proxyCallbacks = relationValue._proxyCallbacks || {};

// Add proxy events to respective parents.
// Only add callback if not defined or new Ctx has been identified.
if (newCtx || (relationValue && !relationValue._proxyCallbacks[relationKey])) {
    // https://github.com/dhruvaray/backbone-associations/issues/145
    if(!relationValue._proxyCallbacks[relationKey]) {
        relationValue._proxyCallbacks[relationKey] = function () {
            return Backbone.Associations.EVENTS_BUBBLE &&
                this._bubbleEvent.call(this, relationKey, relationValue, arguments);
        };
    }
    relationValue.on("all", relationValue._proxyCallbacks[relationKey], this);
}

Line 367:

//Distinguish between the value of undefined versus a set no-op
if (attributes.hasOwnProperty(relationKey))
    this._setupParents(attributes[relationKey], this.attributes[relationKey], relationKey);

Line 479:

//Maintain reverse pointers - a.k.a parents
_setupParents: function (updated, original, relationKey) {
    // Set new parent for updated
    if (updated) {
        updated.parents = updated.parents || [];
        (_.indexOf(updated.parents, this) == -1) && updated.parents.push(this);
    }
    // Remove `this` from the earlier set value's parents (if the new value is different).
    if (original && original.parents.length > 0 && original != updated) {
        original.parents = _.difference(original.parents, [this]);
        // Don't bubble to this parent anymore
        original._proxyCallbacks[relationKey] && original.off("all", original._proxyCallbacks[relationKey], this);
    }
},

Line 661:

// Call this if you want to set an `AssociatedModel` to a falsy value like undefined/null directly.
// Not calling this will leak memory and have wrong parents.
// See test case "parent relations"
cleanup:function (options) {
    options = options || {};

    _.each(this.relations, function (relation) {
        var val = this.attributes[relation.key];
        if(val) {
            val._proxyCallbacks[relation.key] && val.off("all", val._proxyCallbacks[relation.key], this);
            val.parents = _.difference(val.parents, [this]);
        }
    }, this);

    (!options.listen) && this.off();
},
chiplay commented 10 years ago

@jdkanani any thoughts on this problem / solution?

jdkanani commented 10 years ago

Hey @chiplay, solution looks promising. I think you should go ahead with primary pull request. So we can try it out at our end.

@dhruvaray what do you think?

chiplay commented 10 years ago

@jdkanani @dhruvaray PR is up: https://github.com/dhruvaray/backbone-associations/pull/146 - closing this ticket and we can continue discuss there.