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

Two tests (1 failing) showing Collection reset/set should both link related objects #320

Merged
merged 1 commit into from Apr 9, 2013

Conversation

@philfreo
Copy link
Collaborator

@philfreo philfreo commented Apr 4, 2013

I got bit by this by changing a foo.fetch() to foo.fetch({ reset: true }) in my app, and relations stopped working properly.

Here are two tests that should both have the same behavior, but the "reset" one fails.

FYI I'm using the 'this.set()' in Address#initialize because I don't want fetchRelated() to run, since I know that there's already an ajax request fired to fetch all "countries"

PaulUithol added a commit that referenced this pull request Apr 9, 2013
Two tests (1 failing) showing Collection reset/set should both link related objects
@PaulUithol PaulUithol merged commit c31ea90 into PaulUithol:master Apr 9, 2013
@PaulUithol
Copy link
Owner

@PaulUithol PaulUithol commented Apr 9, 2013

Hmm.. I'm not sure about this one. One the one hand, I wouldn't call it 'desired' behavior per se. However, it is very much the expected behavior:

  • Collection.reset calls Collection.add
  • Collection.add sets it's default options; add: true, merge: false, remove: false
  • Collection.add calls Collection.set
  • the model doesn't get updated.

We could set merge: true as a default option in reset, but I'm wondering if that behavior would be better? You could also change the call to countries.reset( [ { id: 'US', name: 'United States' } ], { merge: true } );.

PaulUithol added a commit that referenced this pull request Apr 9, 2013
Revert "Merge pull request #320 from elasticsales/collection-set-vs-reset"

This reverts commit c31ea90, reversing
changes made to 9b52eb2.
@PaulUithol
Copy link
Owner

@PaulUithol PaulUithol commented Apr 9, 2013

Okay, I've made this change. For BBR, merging on reset is probably the most common use case; if it's not desired, you can pass merge: false.

@philfreo
Copy link
Collaborator Author

@philfreo philfreo commented Apr 9, 2013

Makes sense - thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants