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.

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

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

Closed
wants to merge 1 commit into from

Conversation

makebbekus
Copy link
Contributor

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

cohuman commented Nov 20, 2012

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

cohuman pushed 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants