AmpersandJS / ampersand-state

Core state management object.
MIT License
141 stars 75 forks source link

State children can't be constructed in advance #108

Closed callumlocke closed 9 years ago

callumlocke commented 9 years ago

It seems you can only set a 'child' using an attributes hash, not an already-constructed instance. Is this a bug?

This is adapted from the readme example:

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

var Hat = State.extend({
    props: {
        color: 'string'
    }
});

var Person = State.extend({
    props: {
        name: 'string'
    },
    children: {
        hat: Hat
    }
});

var redHat = new Hat({color: 'red'});

console.log(redHat.color); // red

var me = new Person({ name: 'Phil', hat: redHat });

console.log(me.hat.color); // undefined
console.log(me.hat instanceof Hat); // true
console.log(me.hat === redHat); // false
callumlocke commented 9 years ago

I've just been reading the code for this module for the first time, and it looks like children is a much simpler concept than I had first imagined. I was hoping for some more flexibility, like being able to switch out one child for another, and events previously registered on the parent (like "change:hat.color") would continue to work.

I think this functionality would be awesome:

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

var Hat = State.extend({
    props: {
        type: 'string',
        color: 'string'
    }
});

var Person = State.extend({
    props: {
        name: 'string'
    },
    children: {
        hat: Hat
    }
});

// construct a couple of hats
var beanie = new Hat({color: 'blue', type: 'beanie'});
var fedora = new Hat({color: 'red', type: 'fedora'});

// phil is born wearing the blue beanie
var phil = new Person({name: 'Phil', hat: beanie});

// log whenever phil changes his hat
phil.on('change:hat', function (x, newHat, oldHat) {
    console.log(
        'Phil took off his ' + oldHat.color + ' ' + oldHat.type +
        ' and put on a ' + newHat.color + ' ' + newHat.type
    );
});

// and log whenever just his hat *color* has changed
phil.on('change:hat.color', function (x, color, oldColor) {
    console.log('Phil\'s hat was ' + 'oldColor' + ', now ' + color);
});

beanie.color = 'yellow';
//=> "Phil's hat was blue, now yellow"

phil.hat = fedora;
//=> "Phil took off his yellow beanie and put on a red fedora"
//=> "Phil's hat was yellow, now red"

beanie.color = 'pink'; // (nothing to log; phil is not wearing it)

fedora.color = 'green';
//=> "Phil's hat was red, now green"

phil.hat = beanie;
//=> "Phil took off his green fedora and put on a pink beanie"
//=> "Phil's hat was green, now pink"

What do you think? I'd be happy to put together a PR of all these changes if you like it.

If you're doubtful about the use cases, I've got a real-life project that would really benefit from this, I can explain why if interested.

wraithgar commented 9 years ago

I think this seems like the kind of behavior you could reasonably expect from state. This is a pretty big addition IMO so we'd probably want to make sure the test coverage of it is pretty good.

callumlocke commented 9 years ago

OK cool. I also had another thought, might be a bad idea but I'm not sure... what about getting rid of children and collections altogether, and handling all nested stuff using props instead?

eg...

var Hat = State.extend({
    props: {
        type: 'string',
        color: 'string'
    }
});

var Person = State.extend({
    props: {
        name: 'string',
        hat: Hat
        // or possibly hat: 'state', friends: 'collection'  etc?
    }
});
wraithgar commented 9 years ago

That would be a wholly separate discussion imo.

callumlocke commented 9 years ago

I agree it's a wider discussion but not a different one. The kind of state nesting functionality I proposed above could be implemented either as an overhaul to children, or as an overhaul to props. I want to see which option would be of more interest before starting on a PR.

wraithgar commented 9 years ago

There is talk (from @nlf I believe) of making arrays of all instantiated Children and Collections show up properly as attributes of state objects so I don't think those are going away. Knowing that you're switching from just simple nested objects to attaching a proper state model is an important one and the current setup helps you keep that delineation in mind.

mmacaula commented 9 years ago

@callumlocke did you try making 'hat' a 'state'? Eg:

var Person = State.extend({
    props: {
        name: 'string',
        hat: 'state'
    }
});

the state datatype is not quite documented well but it might allow you to run the code you have above and still work?

callumlocke commented 9 years ago

Oh I didn't know that was possible, I'll give it a try Le mar. 18 nov. 2014 à 23:14, Mike Macaulay notifications@github.com a écrit :

@callumlocke https://github.com/callumlocke did you try making 'hat' a 'state'? Eg:

var Person = State.extend({ props: { name: 'string', hat: 'state' } });

the state datatype is not quite documented well but it might allow you to run the code you have above and still work?

— Reply to this email directly or view it on GitHub https://github.com/AmpersandJS/ampersand-state/issues/108#issuecomment-63563941 .

callumlocke commented 9 years ago

This discussion is old and might have become irrelevent after recent changes. Please reopen if it's still relevant.