AmpersandJS / ampersand-state

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

{ parse: true } not passed to children #106

Open jackboberg opened 9 years ago

jackboberg commented 9 years ago

I am not sure if this is expected behavior.

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

var A = State.extend({
  props: { n: 'number' },
  parse: function (attrs) {
    attrs.n = parseInt(attrs.n, 10);
    return attrs;
  }
});

var B = State.extend({
  children: { a: A }
});

// parses string to int
var a = new A({ n: '1' }, { parse: true });

// throws TypeError
var b = new B({ a: { n: '1' } }, { parse: true });
latentflip commented 9 years ago

Hey @jackboberg!

Yeah, that doesn't look correct, good spot. It's sort of related to this https://github.com/AmpersandJS/ampersand-state/issues/92 too.

The reason this is happening is: when we initialize a parent model we:

Since parse: true is ignored in a set() and is only checked and run during initialize, the child model isn't being initialized properly.

I'm not totally sure of the best solution to this immediately, but we definitely need to fix it, as it will be affecting normal server returned results too.

jackboberg commented 9 years ago

Thanks for the feedback. Can you give any insight as to why parse: true is ignored in a set()? See also: [https://github.com/AmpersandJS/ampersand-state/issues/84]

latentflip commented 9 years ago

@jackboberg it's legacy from backbone. They had some discussion about adding that behaviour but it never got merged: https://github.com/jashkenas/backbone/pull/2636/files.

On potential challenge I can see of adding it, is that the required parsing is arguably bound to the server representation of the data: i.e. a custom parse perhaps doesn't make sense in general - it's specifically to deal with data specifically formatted in some way by the server. Though, that argument isn't quite relevant to what we need here.

jackboberg commented 9 years ago

That makes sense, and I agree this is a bit of a tangent. But, I feel like since it defaults to false allowing a user to explicitly do a .set(attrs, { parse: true }) only adds flexibility.

In my uses running parse when it's not needed would not be harmful, just wasted effort. Are there examples where a 'good practice' use of parse could be an issue if it was run unexpectedly?

vwasteels commented 9 years ago

Hey ! Is there a workaround for this ? I tried your pull request but I get a Uncaught TypeError: Cannot read property 'xxx' of null , it seems that attrs is not passed correctly to the parse method. Thanks !

jackboberg commented 9 years ago

I just rebased (my PR was a bit out of date). The tests still pass, not sure why you would get that error.