Instances of a child get ignored on construction

[JaapRood]

Consider the following (edit / run on requirebin here)

var AmpersandState = require('ampersand-state');

var Smile = AmpersandState.extend({
  props: {
    width: 'number' 
  }
});

var Person = AmpersandState.extend({
  props: {
    foo: 'string'
  },
  children: {
    smile: Smile
  }
});

var bobsSmile = new Smile({ width: 100 });
var bob = new Person({ smile: bobsSmile });

// in Person.prototype._initChildren a new instance gets created instead of using the existing one
alert(""+ bobsSmile.cid + " !== " + bob.smile.cid);
// state1 !== state3

// causes heartbreak :(
bobsSmile.width = -100;

alert("" + bobsSmile.width + " !== " + bob.smile.width);
// -100 !== undefined

// there is a way for our friend to find back where his smile was, but why leave him depressed?
bob.smile = bobsSmile;

alert("" + bobsSmile.cid + " === " + bob.smile.cid);
// state1 === state1

Maybe this has done by design, but I can't help but think it's confusing and not the behaviour we're looking for. Ideally there should only be one instance of every resource representation across the app, otherwise keeping everything in sync would become a pain.

[JaapRood]

It's worth mentioning that in the example above the event bubbling between the parent and child is broken once you assign the model instance after constructing. Children are set directly in _initChildren and attached to the instance like so:

for (child in this._children) {
  this[child] = new this._children[child]({}, {parent: this});
  this.listenTo(this[child], 'all', this._getEventBubblingHandler(child));
}

We should probably set a getter / setter so that when a new model gets assigned we stop listening to the old model and start listening to the new one.

[JaapRood]

I created a quick and dirty mixin that uses the a prop of the type state, as that seems to provide the wanted behaviour (thanks for the tip on twitter @HenrikJoreteg). The mixin overrides the set method to create an instance of a child when none has been set yet.

While being quite familiar with Backbone source I'm pretty new to Ampersand and although it's similar I'd like some feedback on whether this would be the right approach before spending time porting it back to ampersand-state and making a pull request :)

[HenrikJoreteg]

Hey, @JaapRood. Sorry for the slow response here.

So, maybe you already discovered this, but we already have a property type of state that does what I think you're wanting here.

So, if you change your Person constructor above to look like this:

var Person = AmpersandState.extend({
  props: {
    foo: 'string',
    smile: 'state'
  }
});

I believe the problem goes away.

So, therein lies the subtle difference between children and using a prop type of state. The ones that are explicitly listed as children are not designed to be swapped out. The reason for having support for both is because prop type state is intended to be used as a watchable property like any other, that can be swapped out, etc.

The children on the other hand allow you to "inflate" them automatically if you're getting nested JSON structures back from an API call for example. They also automatically serialize back to nested JSON on the way out. This is very useful for APIs that are structured in this way.

Also, "empty" children always get instantiated when the parent model does so they can be expected to always exist.

This sounds like another case where we need to improve the documentation.

[HenrikJoreteg]

Here are the relevant tests for state properties: https://github.com/AmpersandJS/ampersand-state/blob/master/test/full.js#L1043-L1150

And for children: https://github.com/AmpersandJS/ampersand-state/blob/master/test/full.js#L818-L1041

[HenrikJoreteg]

I filed a bug to write a guide addressing this difference: AmpersandJS/ampersandjs.com#78

Please re-open this if there's still an issue. Thanks for your very detailed bug filing, seriously. Much appreciated.

[JaapRood]

That's exactly what I figured out, but what I don't quite understand is why make the distinction between the two at all? In a project I'm using the following mixin (also adds support for polymorphism, but the main gist is still there):

var State = require('ampersand-state'),
    _ = require('lodash');

module.exports = function(childModels) {
    if (!childModels) childModels = {};

    var props = {};

    _.each(childModels, function(def, name) {
        props[name] = 'state';
    });

    return {
        props: props,

        set: function(key, value, options) {
            // Replicating a bit of the source so we can support all set 
            var attrs;

            // Handle both `"key", value` and `{key: value}` -style arguments.
            if (_.isObject(key) || key === null) {
                attrs = key;
                options = value;
            } else {
                attrs = {};
                attrs[key] = value;
            }

            options = options || {};

            var model = this;
            // if a child isn't set yet and only an object is given, create an instance
            _.each(childModels, function(def, name) {
                // normalise the child definition
                var childConstructor;

                if (_.isFunction(def)) {
                    childConstructor = def;
                } else {
                    childConstructor = def.getConstructor.call(model);
                }

                if (!this[name] && attrs[name] && !(attrs[name] instanceof childConstructor)) {
                    attrs[name] = new childConstructor(attrs[name], options);
                }
            });

            // defer rest to the original implementation
            return State.prototype.set.call(this, attrs, options);
        }
    };
};

I use that like so:

var childModels = require('child-models-mixin');

var Smile = AmpersandState.extend({
  props: {
    width: 'number' 
  }
});

var Person = AmpersandState.extend(childModels({
  smile: Smile
}),{
  props: {
    foo: 'string'
  }
});

It really is a hack and I don't suggest copying that straight into source of course, but it seems to merge the two concepts together: it's children as props with type 'state' that also get inflated if a hash is given instead of an instance of the child. Also: having a child as a prop with type 'state' allows you to change the model for another one, while defining it using a 'children' definition, as soon as you swap out the instance (which you should be able to do, it being just a prop) it breaks.

It doesn't cover that there is always an instance available, the prop could be null, but if there is some way we can get that to work I think merging the two is by far preferable. Personally I get around by setting a default value for the child prop (by using 'state' type) if I need guarantee of existence and it seems to work out just fine (although let me know if you have a reason for concern there).

Add to that the confusion having two of these different but closely related concepts around, I think this is worth considering.

[JaapRood]

@HenrikJoreteg Github won't let me re-open this issue, so unless you want me to repost the issue you'll have to do it 😄

[HenrikJoreteg]

So, @latentflip and I struggled back and forth on this issue for a while. I'm trying to recall exactly why we went this route. I wonder if @latentflip can recall.

[HenrikJoreteg]

I agree that the distinction is confusing. Perhaps we can eliminate the declarative children.

I think part of the reason we did it this way was because we didn't used to support having default values for properties be functions that returned instances. Now that we have that it's entirely possible the purpose of having children is moot.

[latentflip]

I think the main difference was whether the child model would be initialised with data parsed out of the json/serialised in outgoing json or not.

It seems like we already have semantics for that with props & session though. So +1 on revisiting removing children & collections.

Philip Roberts
&yet

On 12 Aug 2014, at 05:36, Henrik Joreteg [email protected] wrote:

I agree that the distinction is confusing. Perhaps we can eliminate the declarative children.

I think part of the reason we did it this way was because we didn't used to support having default values for properties be functions that returned instances. Now that we have that it's entirely possible the purpose of having children is moot.

—
Reply to this email directly or view it on GitHub.

[JaapRood]

I get that having 'children' and 'collections' props do clean up the definition of the props. You can keep them around simply as a small abstraction on top of the props and save having to define everything manually, sort of like the mixin I posted above. Don't have too great insights on how feasible that actually is though

[HenrikJoreteg]

Yeah, I do like the semantics of having them listed separately, but like you said, we could still support that syntax.

[jrmyio]

This seems to be related to my issue: #43
Depending if still needed, collections is not in the docs atm: AmpersandJS/ampersandjs.com#79
Also, here is my opinion on this: AmpersandJS/ampersand-collection#11 (comment)

Also, is this 'state' also supposed to be used with collections?

[HenrikJoreteg]

I think most of this issue can be considered addressed with the documentation changes in PR above.

It's possible we need to make further changes to how these cases are handled. But I think the explanation is at least a good start. Re-open if this fails to address these concerns.