nested attr() call on a model with List attributes blows away existing List #160

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@makebbekus
Contributor

makebbekus commented Nov 18, 2012

#151 fixed this issue with simple objects, but the test here shows it's still a problem when the attributes plugin is involved.

The failure is the same as before in that a new list (with a new _cid) is made.

I'm not quite sure how to address this one, so I'm just submitting the failing test.

Thanks!

@ghost ghost assigned daffl Nov 19, 2012

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Nov 19, 2012

Contributor

Yes, this is because of the attributes plugin. When setting members, EmptyListTest.User.models will be called, which returns a new Observable list every time.

I think we could change .models and the attributes plugin so that it removes all elements and adds the new ones if the current attribute is a can.Observe.List already. That should solve the problem right?

Contributor

daffl commented Nov 19, 2012

Yes, this is because of the attributes plugin. When setting members, EmptyListTest.User.models will be called, which returns a new Observable list every time.

I think we could change .models and the attributes plugin so that it removes all elements and adds the new ones if the current attribute is a can.Observe.List already. That should solve the problem right?

@daffl daffl closed this in 0774dca Nov 19, 2012

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Nov 19, 2012

Contributor

Ok, I pushed a fix. The attributes plugin now clears and updates an existing list, your test case is passing.

Contributor

daffl commented Nov 19, 2012

Ok, I pushed a fix. The attributes plugin now clears and updates an existing list, your test case is passing.

@cohuman

This comment has been minimized.

Show comment
Hide comment
@cohuman

cohuman Nov 20, 2012

Contributor

Awesome, thanks! This should fix our problem. We'll check it out.

Contributor

cohuman commented Nov 20, 2012

Awesome, thanks! This should fix our problem. We'll check it out.

cohuman added a commit to cohuman/canjs that referenced this pull request Dec 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment