Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How important is it to have an explicit reverse relation support? #51

Closed
dhruvaray opened this issue Jul 10, 2013 · 21 comments
Closed

How important is it to have an explicit reverse relation support? #51

dhruvaray opened this issue Jul 10, 2013 · 21 comments
Assignees

Comments

@dhruvaray
Copy link
Owner

BB Associations supports reverse (inverse) relationships implicitly - via the parents property. Do users of BB Associations see a value in supporting explicit reverse relations - in the style of Backbone-relational?

Zoo = Backbone.RelationalModel.extend({
    relations: [{
        type: Backbone.HasMany,
        key: 'animals',
        relatedModel: 'Animal',     
        reverseRelation: {
            key: 'livesIn',
            includeInJSON: 'id'
        }
    }]
});
Animal = Backbone.RelationalModel.extend({
});

can also be simply modeled as

Zoo = Backbone.AssociatedModel.extend({
    relations: [{
        type: Backbone.Many,
        key: 'animals',
        relatedModel: 'Animal',     
    }]
});
Animal = Backbone.AssociatedModel.extend({
    relations: [{
        type: Backbone.One,
        key: 'livesIn',
        relatedModel: 'Zoo',        
    }]
});

Would be nice to see an example of what explicit reverse relation support could bring to the table.

EDIT : 19th July

Also, reverse relations like the one defined in BB-Relational can cause ambiguity and coupling between model definitions.

For example

Animal = Backbone.RelationalModel.extend({
});

Zoo = Backbone.RelationalModel.extend({
  relations: [{
        type: Backbone.HasMany,
        key: 'animals',
        relatedModel: 'Animal',     
        reverseRelation: {
            key: 'livesIn',
            includeInJSON: 'id'
        }
    }]
});


House = Backbone.RelationalModel.extend({
    relations: [{
        type: Backbone.HasMany,
        key: 'pets',
        relatedModel: 'Animal',     
        reverseRelation: {
            key: 'livesIn',
            includeInJSON: 'id'
        }
    }]
});

var cat = new Animal({name:"kitty", livesIn:{name:"my sweet home"}});

cat.get('livesIn') instanceof House //true


var lion = new Animal({name:"ferocious", livesIn:{name:"NY ZOO"}});
lion.get('livesIn') instanceof Zoo //false!
lion.get('livesIn') instanceof House //true!
@sdeleuze
Copy link

I am currently porting a very big app from BB relational to BB associations. We made very heavy use of reverseRelation so I will let you know if I have some use case where I am blocked with current BB assocations implementation.

I will work on this next week.

@ghost ghost assigned dhruvaray Jul 13, 2013
@dhruvaray
Copy link
Owner Author

I would recommend you use the edge version. We are about to release v0.5.1. You can read about the upcoming features in the changelog

With that out of the way:

Thanks for your appreciation!

I am excited to hear about your real-world experiences during your port to BB-Associations and whether reverse relations can be avoided! If it goes successfully, it would be nice if you could share your recipes and experiences - so that I could include it in our documentation.

@sdeleuze
Copy link

I still have some regressions due to BB Relational cache removal, but migration seems to be mostly successful. Your sample on this issue was really helpful, and should be integrated in documentation if it is not.

I will keep you inform when all migration bugs will be fixed.

BB associations is a pleasure to use so far.

@sdeleuze
Copy link

We plan to integrate BB Assocations in our upcoming of RESThub stack (http://resthub.org/), can you tell me when 0.5.1 release is expected ?

@dhruvaray
Copy link
Owner Author

From the perspective of code, we are ready. I just need to update the CDNs and the documentation. So I would say a couple of days - at most.

I am really happy to hear that the migration has been mostly smooth so far! Based on your experiences, I would like to put in a upgrading from BB-Relational section. The example in this issue and whatever problems you share will find it's way into that documentation.

@dhruvaray
Copy link
Owner Author

v0.5.1 is now released. The documentation has been updated with your suggestions. I am going to wait till you finish your port to BB-Associations before I write a "porting from BB-Relational" section. It would be nice if I could also link to http://resthub.org on our website. It's always great to see a complete stack implementation!

@sdeleuze
Copy link

Nice, thanks.

I have updated our snapshot documentation at http://resthub.org/reference/snapshot/backbone-stack.html, we just have access problem to the website as described on #53 issue.

@sdeleuze
Copy link

I think we have found an issue while porting our big application from Backbone Relational to Backbone Associations.

The issue is the following, I have a Parent - Child relation like following :

var Record = Backbone.AssociatedModel.extend({
    relations: [{
                      type : Backbone.Many,
                      key: 'childrenMinders',
                      relatedModel: ChildMinder,
                      collectionType : ChildrenMinders
                }]
}

var ChildMinder = Backbone.AssociatedModel.extend({
     relations : [{
            type: Backbone.One,
            key: 'record',
            relatedModel: Record
      }]
}

When we serialize Record we have something like this

{ id: 2,
  name: 'test1',
  childrenMinders: [
    {id: 1, type: 'test1'},
    {id: 2, type: 'test2'},
    {id: 3, type: 'test3'}]
}

Based on our server requirement, we need something more like this, with the backreference from ChildMinder to Record provided (only this id) :

{ id: 2,
  name: 'test1',
  childrenMinders: [
    {id: 1, type: 'test1', record: { id: 2},
    {id: 2, type: 'test2', record: { id: 2}},
    {id: 3, type: 'test3', record: { id: 2}}]
}

Is it currently possible with Backbone Associations or any improvement possible to support such use case ?

@dhruvaray
Copy link
Owner Author

@sdeleuze : I just added a test case - commit : 428ec34 - to demonstrate how to handle your scenario with BB-Associations. Let me know if you have any doubts or comments.

@sdeleuze
Copy link

sdeleuze commented Aug 1, 2013

@dhruvaray Thanks for your test case. It works but I am a little puzzled by the need to add the findRecordById logic to each model where I have this kind of relations.

Could you explain when occur different cases :

  1. The case where val is an instance of Models.Record is pretty obvious
  2. When should we have id = val ?
  3. When should we have id = val.id ?
    I just need more details to understand why we have 3 different cases to manage ...

Anyway, I understand that since we do not have a store like in BB relational, we have to implement Record retrievable, but for simple case (1), isn't it something that could be manage by Backbone Associations automatically without exposing "internals" like the one implemented in findRecordById + map attribute definition ?

dhruvaray added a commit that referenced this issue Aug 2, 2013
@dhruvaray
Copy link
Owner Author

I put conditions 2 & 3 to just demonstrate various serialization options. You could trim as per your requirements

  • id = val.id : Will handle this kind of object graph serialization
{ id: 2,
  name: 'test1',
  childrenMinders: [
    {id: 1, type: 'test1', record: { id: 2},
 }
  • id=val : Will handle this kind of object graph serialization
{ id: 2,
  name: 'test1',
  childrenMinders: [
    {id: 1, type: 'test1', record: 2},
 }
  • val instanceof Models.Record : To handle direct assignments.

Because a map function has been specified, the framework will call the map function to transform the passed value before proceeding. Hence the need to handle all the scenarios. However, practically speaking, you would typically be passing only ids around for Models.Record. (As it is probably a shared application entity and referred in multiple places) Consequently, the test case can be simplified to

findRecordById:function (val) {
   return _.findWhere(MyApp.Context.provinceRecords, {id:val.id});
}

See 9a96972 for the updated test case.

I hope this makes things clear.

dhruvaray added a commit that referenced this issue Aug 8, 2013
@dhruvaray
Copy link
Owner Author

@sdeleuze : Thinking a bit more about your scenario, we don't need to define a mapper function. Just setting options.merge = false during rootmodel.save() would handle your scenario correctly. See the re-written version of your scenario in commit 7893855. Let me know if that helps!

dhruvaray added a commit that referenced this issue Aug 9, 2013
@dhruvaray
Copy link
Owner Author

@sdeleuze : I just now made a change - b8257a8 - which should handle the common (cyclic) scenario transparently.

With this change, you don't need to over-ride the toJSON method for cycle scenarios or pass options to save(/fetch/set) methods.

With this change, you can use BB-Associations to do exactly what you wanted here - without any special application handling.

@dhruvaray
Copy link
Owner Author

@sdeleuze, others : As an FYI : Our recently released Backbone Eye - a tool to analyze and understand Backbone application behavior without debugging Javascript - also plays well with Backbone Associations (and other nested frameworks as well). The above example would graphically pan out like this in our tool.

childminders

Would love to see folks take this tool for a spin!

@dhruvaray
Copy link
Owner Author

Recipe gist here and updated recipe list in documentation.

@sdeleuze
Copy link

Thanks a lot, I am going to try this improvement

@mattcroberts
Copy link

I would find implicit reverse relations quite useful, in my app I use requirejs to load each "class", when defining a reverse relation each relation definition needs to specify the related model which causes a circular dependency.

I can provide an example if needed.

@dhruvaray
Copy link
Owner Author

@mattcroberts : You can specify the circular dependency in requirejs as shown in #74. Is that the only reason you would want implicit reverse relations?

@mattcroberts
Copy link

I've created a gist to demonstrate what I'm trying to do and with the approach you linked to which hasn't worked for me.

https://gist.github.com/mattcroberts/6921581

@dhruvaray
Copy link
Owner Author

Here's the working version of your gist : https://gist.github.com/dhruvaray/6929867
With require.js, your functions would have to be like this for cycles

return require("Person"); //and  return require("House"); in `House` and `Person` respectively. 

@ronen
Copy link
Contributor

ronen commented Dec 20, 2013

Since I've been mentioning reverse relations in #88 and #91, I figured I'd add my thoughts to this thread.

  • Why do I want reverse relations? I've got various cases with a One-to-Many relationship where I start from the One model and render each of its Many models so I need the forward Many relationship. But properly rendering each of the Many models requires accessing some attributes of the One model, so I need the reverse relationship.
  • Where do I want reverse relations? Really, everywhere. I don't want to have to think about it, I should always be able to traverse backwards and forwards through my object graph.
  • Why do I want it implicitly rather than explicitly? Three reasons:
    • Having to explicitly declare each "half" relation twice in two different files isn't quite DRY enough for me.
    • I don't want to have to worry about setting the parent ID's explicitly in the child when I add the child to the parent, I'd like that to happen automatically.
    • I could easily imagine that performance improvements could be gained by having bb-associations recognize and handle two-way relationships, rather than having to have twice the number of one-way relationships.

The question about ambiguity & coupling is a good one. Actually, I think ambiguity's not a huge issue, that can be handled through appropriate warnings should an attempt be made to define two relationships with the same name. Coupling is admittedly somewhat disturbing; but I think the coupling is there because semantically the models really are coupled.

On that last point, I'm somewhat thinking these days that the declaration of a relationship between to models shouldn't be in either of the model's definitions. In fact in my own app I'm on my way to making a bidirectional-relation definition along the lines of:

BB.Associations.oneToMany({
        one: {
              model: Zoo,
              key: 'zoo',
       },
      many: {
              model: Animal,
              key: 'animals',
      }
})

which in turn does:

 Zoo.prototype.relations.push({ type: BB.Many, relatedModel: Animal, key: 'animals' })
 Animal.protype.relations.push({ type: BB.One, relatedModel: Zoo, key: 'zoo' })

and also with some hooks to create event listeners that will update one direction from the other automatically, plus setting up JSON serialization, store-lookup/mapping, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants