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.

Sign up for GitHub

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

Jump to bottom

{ parse: true } not passed to children #106

Open
jackboberg opened this issue Nov 17, 2014 · 6 comments · May be fixed by #109
Open

{ parse: true } not passed to children #106

jackboberg opened this issue Nov 17, 2014 · 6 comments · May be fixed by #109
Labels
bug

Comments

@jackboberg
Copy link
Contributor

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 latentflip added the bug label Nov 17, 2014
@latentflip
Copy link
Contributor

Hey @jackboberg!

Yeah, that doesn't look correct, good spot. It's sort of related to this #92 too.

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

  • Initialize the parent with the full props, and options
  • Initialie the child with empty props, and only {parent: true} as the options
  • Then do a set on the child model with the child props, and the full options

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
Copy link
Contributor Author

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

@latentflip
Copy link
Contributor

@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
Copy link
Contributor Author

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?

@jackboberg jackboberg linked a pull request Nov 21, 2014 that will close this issue
Open
@vwasteels
Copy link

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
Copy link
Contributor Author

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

@latentflip latentflip mentioned this issue Jan 19, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow passing { parse: true } to set jackboberg/ampersand-state
3 participants
@latentflip @jackboberg @vwasteels