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

Added support for self-referential relationships between models when using AMD / RequireJS #127

Closed
wants to merge 1 commit into from

Conversation

dpwolf
Copy link

@dpwolf dpwolf commented May 16, 2012

If special keyword string "self" is used in relatedModel, use the current model as the relatedModel.

Added test to show this works.

…rent model as the relatedModel.

Added test to show this works.
@PaulUithol
Copy link
Owner

Uhm.. self-referential relationships have always been supported. What's wrong with just using the name of the model itself?

@Toobian
Copy link

Toobian commented May 21, 2012

If you want use AMD with RequireJS for example, you can't use global scope to store models. So model should be something like:

var A= Backbone.RelationalModel.extend({
    relations: [
        {
            type: Backbone.HasMany,
            key: 'child',
            relatedModel: A,
            reverseRelation: {
                key: 'parent'
            }
        }
    ],
});

But you can't use A because it's not define.

@dpwolf
Copy link
Author

dpwolf commented May 21, 2012

Yes, thanks @Toobian, I should have explained. When loading relational models with Require.js they are not globally defined so can't be accessed via the text string model name method.

@DouweM
Copy link
Collaborator

DouweM commented May 21, 2012

I'll be using AMD in the near future as well, so this gets an 👍 from me!

@PaulUithol
Copy link
Owner

I don't think using some "magic" word syntax is the right approach here, as the problem appears to be broader than that (see #57). I'm not sure how scopes actually do work in that environment, but isn't there some (named) scope or scope object that could be added to exports in backbone-rel? If so, couldn't this be an array of scope objects that will be searched for matching models?

@Toobian
Copy link

Toobian commented May 22, 2012

I don't really understand what you mean. But I tried something that seemed good and it failed.
If I resume my example with some changes.

var tmp = {};
tmp.A= Backbone.RelationalModel.extend({
    relations: [
        {
            type: Backbone.HasMany,
            key: 'child',
            relatedModel: tmp.A,
            reverseRelation: {
                key: 'parent'
            }
        }
    ],
});

And after you can exports tmp.A. For me, tmp was like window scope but backbone-rel don't recognize tmp.A. So now I don't understand how do backbone-rel and why my example doesn't work :/

@PaulUithol
Copy link
Owner

Well, I was trying to explain how we may be able to create something that does work (although I really don't know the specifics of the scope and security model used in node.js et al.). So it couldn't work yet anyway.

What I'm thinking of is more like the following:

var tmp = {};
Backbone.Relational.store.addModelScope( tmp );

tmp.A= Backbone.RelationalModel.extend({
    relations: [
        {
            type: Backbone.HasMany,
            key: 'child',
            relatedModel: 'A',
            reverseRelation: {
                key: 'parent'
            }
        }
    ],
});

Where the implementation of Backbone.Relational.store.addModelScope could be as simple as

addModelScope: function( scope ) {
    exports.push( scope );
}

getObjectByName would then have to be updated to treat exports as an array, instead of a single value. Does anyone have more insight into whether this could work, given the constraints on scope and the security model in such environments?

@Toobian
Copy link

Toobian commented May 31, 2012

After look your proposition, I think it's good solution. I don't found better one.

@j-5-s
Copy link

j-5-s commented Jun 17, 2012

+1 for merging this.

PaulUithol added a commit that referenced this pull request Jun 19, 2012
@PaulUithol PaulUithol closed this Jun 19, 2012
@PaulUithol
Copy link
Owner

Okay, please give it a try with the current version. Really hope this does the trick. Example from the tests:

var models = {};
Backbone.Relational.store.addModelScope( models );

models.Book = Backbone.RelationalModel.extend({
    relations: [{
        type: Backbone.HasMany,
        key: 'pages',
        relatedModel: 'Page',
        createModels: false,
        reverseRelation: {
            key: 'book'
        }
    }]
});
models.Page = Backbone.RelationalModel.extend();

var book = new models.Book();
var page = new models.Page({ book: book });

ok( book.relations.length === 1 );
ok( book.get( 'pages' ).length === 1 );

@j-5-s
Copy link

j-5-s commented Jun 19, 2012

Works great. Thank you!

@JaapRood
Copy link

JaapRood commented Jul 2, 2012

It works but I'm running into trouble with naming conflicts now. I've got more models called "Page" in different scopes, someting that isn't uncommon with working with AMD modules. Also: how efficient will this be when you add heaps of scopes?

In my experience, a better solution would be to let relationalModel accept a function that returns the model. It's actually something I've tried and works perfectly. It harnesses Javascript's expressive power and was actually suggested by @jrburke in #57. While using this, I've only ran into some problems with RelationalModel.fetchRelated(), but I'm not sure if that's related (am still a bit new to the code).

@sampsasaarela
Copy link

In my experience, a better solution would be to let relationalModel accept a function that returns the model.

+1

I totally agree. Current addScope method is not the best solutions and I ran into naming conflicts also.

@Karabur
Copy link

Karabur commented Aug 28, 2012

Also +1. Just came here to post aboit it and saw that I not the first one.
This approach with having a 'scopes' doesnt fit to module practice well. And AMD and CommonJS. It became a some kind of 'global' place where all this fine isolation of modules start 'leaking'. And also it is a common way to have one class (model) in each module, so we have a growing set of scope objects with only one property each.

@andrewferk
Copy link
Contributor

+1 for having relationalModel accept a function as name conflicts between scopes exist.

@al
Copy link

al commented Dec 10, 2012

So using the example above I can get a self-referential relationship setup, however the collectionType doesn't seem to be respected. The models are storing their associations in a regular Backbone.Collection, despite being told to use a custom collection class. I'll try fiddle an example later, but has anyone else encountered this/got a quick solution?

@nicosantoro
Copy link

wrt the following code, Backbone tries to fetch data for each model even if we add "autoFetch:false" explicitely.
With another relational model that follows the very same scheme but does not use self-referential relationship we don't have this problem.
Can you help?

define([
  'exports',
  'underscore',
  'base/module',
  './collection'
], function(exports, _, base, collection) {
  'use strict';

  var models = {};
  Backbone.Relational.store.addModelScope(models);

  models.Model = exports.Model = base.RelationalModel.extend({
    parse: function(response) {
      response.id = SOME_FUNCTION(some_argument);
      return response;
    },
    relations: [{
      collectionType: collection.Collection,
      key: 'events',
      keySource: 'nodes',
      relatedModel: 'Model',
      parse: true,
      reverseRelation: {
        key: 'event',
        includeInJSON: 'id'
      },
      type: Backbone.HasMany
    }]
  });
});

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

Successfully merging this pull request may close these issues.

None yet