Circular dependencies model -> children -> collection with same model

[dazld]

I've been trying to implement a circular dependency (https://gist.github.com/dazld/e6a04574ef10e81fe9ab) - but as written, this doesn't appear to work.

I've tried to replace the model property of the collection with a function that requires and instantiates, but without success.

I also tried using the collections property, but this means that events from the collection/child will not bubble up, which isn't so useful.

Is this something that ampersand just cannot support, or is there a technique which works..? I'm using webpack as a compiler.

[latentflip]

Circular requires are a fundamental browserify limitation. So first step would be to put them in the same file.

You still then have the circular definition issue (which do you define first).

One option is to do something like this:

var Model = require('ampersand-model');
var Collection = require('ampersand-collection');

var Items = Collection.extend({
    model: function (props, options) {
        return new Item(props, options);
    }
});

var Item = Model.extend({
    props: {
        'entry':'string',
        'created_at':'date'
    },
    children: {
        items: Items
    }
});

//Or whatever here:
module.exports = {
    Model: Item,
    Collection: Items
}

[prust]

@latentflip:

Circular requires are a fundamental browserify limitation

Do you know of any documentation, github issues or blog posts about this?

The only things I found made it sound (to me, I'm not very familiar w/ Browserify yet) like Browserify could handle circular requires:

https://github.com/substack/node-browserify/tree/master/test/cycle https://github.com/substack/node-browserify/tree/master/test/circular

[latentflip]

@prust oh, I'm probably misremembering then, sorry (I solved a similar thing once here: https://gist.github.com/latentflip/080a14052edf067654e7).

@dazld just realized I possibly solved this better in ^ too:

var NodeList = AmpersandCollection.extend({
});

var Node = AmpersandState.extend({
  props: {
    id: 'string',
    name: 'string'
  },

  collections: {
    children: NodeList
  }
});

NodeList.prototype.model = Node;

[prust]

@dazld: Sorry, it just dawned on me that webpack is an alternative to browserify, not something you're using in addition. Circular requires are supported in webpack too, but they have the same limitations as in browserify & Node, which is the problem you're running into. See http://nodejs.org/api/modules.html#modules_cycles for a detailed explanation.

@latentflip's example solves this is by first doing an empty extend() in order to expose the reference to the class so it can be used by the other module, and then later applying what was in the extend (setting model on the prototype).

You can apply this same technique in your context -- you shouldn't need to merge them into one file. It would look like this (items.js):

var Collection = require('ampersand-collection');
var MyCollection = Collection.extend({});
module.exports = MyCollection; // export the right variable before any cyclical require()s

var Item = require('../models/item');
MyCollection.prototype.model = Item;

Edit: Fixed Collection.prototype.model bug that @dazld notes below.

The key is to do your exports before doing any other requires -- that allows the dependencies to get a valid reference to Collection when they require it.

With the above adjustment to items.js, everything should work so long as everyone require()s items.js before item.js. If some code requires them in the opposite order -- or just requires item.js -- then you'll need to apply the same treatment to item.js (doing the export before requiring items.js). But this gets annoying because you have to duplicate the functionality of extend(). @latentflip will know better than I if there is an easy way to re-use the functionality in extend() after you've already done an empty extend({}).

This is an upside to @latentflip's technique of just putting them in the same file: that way you control the order in which they're defined and you can always define the collection first, saving yourself some hassles.

[dazld]

@latentflip @prust thanks for the detailed and interesting replies!

It seems like this is all a limitation of the commonJS module system as implemented in node, browserify, webpack etc - in that it will supply a partially evaluated object when it encounters cyclical dependencies, causing the problems when referenced (thats my takeaway from the documentation).

I'm going to play with both of those patterns you suggest, and come back with something more intelligent after trying it. For simple stuff, the single file approach seems at first glance a winner, but will try.

One point @prust - you suggest Collection.prototype.model = Item; as a solution, which looks like a typo. That would actually put Item on the model property of the prototype for all collections that don't explicitly set one in extend which is probably not such a great idea ;)

[prust]

@dazld:

looks like a typo

Right, I meant to put it on the new custom collection class -- which didn't have a variable (besides module.exports). I updated the example to set it to MyCollection and updated that prototype. Thanks!

[dazld]

I've done a bit more testing, and found a couple of interesting things using the single file approach:

1. When a model has that collection as a child, instantiating the model results in a call stack overflow
2. When a model has a collection as part of the undocumented collections property (as noted in https://github.com/AmpersandJS/ampersand-state/issues/71), it works but reading the code, seemingly without event bubbling..?
3. When overriding the model property of a collection with a function, you are required to implement isModel too, not just for polymorphic cases as suggested in the documentation (probably obvious!).

here's the gist: https://gist.github.com/dazld/a8f3dae1721127786bdb

in both.js replace the children property of Item with collections to avoid the call stack overflow.

(no compilation, just outputting to console from node)

[dazld]

I suppose this is turning more into a question about semantics between children and collections - the circular dependency thing is pretty much resolved through the techniques mentioned above. I think it was harder to figure out because of not knowing exactly what is happening under the hood.

Having that particular collection as a child does seem to be the big problem for now, in that it throws.

[latentflip]

@dazld yes, while poorly documented, children should be "state/model" objects and collections should be "collections".

[dazld]

@latentflip thanks, closing as think we've bottomed this one out :)

[jdrucza]

Since the discussion here was the best (only) reference that helped me solve this issue for a slightly more complicated hierarchy with circular dependencies, thought I'd share a slightly modified version that seems to work well and a second, albeit, untested solution that is kind of neat for simple hierarchies:

1. Use a derived property for the child/associated collection and define the Model first:

var Model = require('ampersand-model');
var Collection = require('ampersand-collection');

var Item = Model.extend({
    props: {
        'entry':'string',
        'created_at':'date'
    },
    derived: {
        items: {
            deps: ['id'],
            fn: function() { 
                new Items( parentId: this.id ); 
            }
        }
    }
});

var Items = Collection.extend({
    model: Item
});

module.exports = {
    Item: Item,
    Items: Items
}

1. Inline the Model definition inside of the collection

var Model = require('ampersand-model');
var Collection = require('ampersand-collection');

var Items = Collection.extend({
    model: Model.extend({
        props: {
            'entry':'string',
            'created_at':'date'
        },
        collections: {
            items: Items
    });
});

module.exports = {
    Item: Items.model,
    Items: Items
}

I'd be interested to hear what anyone thinks but the first is working well for us at the moment.

[prust]

@jdrucza (cc @latentflip, @michaelsmitha): A couple weeks ago I realized there was an easier way to avoid circular dependencies: use utils.inherits() instead of Backbone/Ampersand's extend() and export the class on the 2nd line.

Backbone/Ampersand's inheritance API forces you do everything in one step:

var SubClass = BaseClass.extend({
  // methods/properties here
});

Node's API is more fine-grained (though admittedly more verbose); consisting of 3 separate steps:

function SubClass() { SubClass.super_.apply(this, arguments); }
// require BaseClass, underscore, etc here
util.inherits(SubClass, BaseClass);
_.extend(SubClass.prototype, {
  // methods/properties here
});

Because Node's API is split into separate steps, it allows you to export the SubClass between the first and second steps, on the 2nd line of the file:

function SubClass() { SubClass.super_.apply(this, arguments); }
module.exports = SubClass;
// require BaseClass, underscore, etc here
util.inherits(SubClass, BaseClass);

Following this simple pattern will eliminate all circular dependency issues -- it even allows you to reference SubClass in BaseClass and vice-versa if you wanted to.

Node's util.inherits() also has a number of other advantages:

1. It is provides a super_ property, unlike Backbone's API (Backbone does have a super property, but it's private & is not intended to be used by anyone but coffeescript; numerous people have written alternatives).
2. It is more inline with the mini-module philosophy. Even though ampersand-class-extend is a separate module under the hood, it gets mixed into the API of whatever classes use it. Providing inheritance as a separate API (util.inherits()), as node does, makes it a little clearer to end-users that, regardless of what inheritance mechanism the module used internally, they are free to use whatever inheritance mechanism they want to write their own derived classes.
3. It allows you to define your SubClass constructors with a named function, which makes a big difference when debugging memory leaks with Chrome's dev tools (in Chrome's Heap Snapshot profiles, and probably other places as well, every Backbone/Ampersand derived class is lumped together as child since that's the variable name used internally by extend(). This makes it harder to effectively use this particular dev tool)
4. It is more general-purpose than Backbone's inheritance and can be used on any class, not just those that inherit from Backbone/Ampersand.
5. It is the de-facto standard for Node, so there are lots of resources, examples & helps available

Isaac Schlueter put node's inherits implementation into a browser-friendly module, https://github.com/isaacs/inherits, and added a fallback for browsers that don't support Object.create(). For browserify, just use var inherits = require('inherits');. It may automatically include it if you require util, but I'm not 100% sure about that.

It may be worth playing with node's inheritance mechanism -- I believe it can be used with ampersand now, without any changes in the core classes. If the ampersand folks like it and want to recommend it, they could remove extend() from the core classes, while providing a module that monkey-patches those modules by mixing it back in, as a migration aid for those who need backwards-compatibility for a while longer.

[prust]

I just remembered that Backbone/Ampersand allows you to specify your constructor with a special constructor property, so there is an easy way to avoid the circular require problem without switching to node's inheritance API (though I still prefer node's API):

function SubClass() {
  BaseClass.prototype.apply(this, arguments);
}
module.exports = SubClass;
// do all your requires here, after export (except BaseClass, which has to be at the top)
BaseClass.extend({
  constructor: SubClass,
  // methods/properties here
});