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

Relationship changes #2576

Merged
merged 6 commits into from
Dec 18, 2014
Merged

Relationship changes #2576

merged 6 commits into from
Dec 18, 2014

Conversation

igorT
Copy link
Member

@igorT igorT commented Dec 11, 2014

This PR makes several changes to relationship handling:

  1. It separates the path through which server side and client side updates happen, which lays the ground work for tracking relationship changes.
  2. When pushing records into the store, it splits the push process into three phases using the runloop:
    a) Push primary key data and attribtues
    b) Turn foreign keys into records
    c) Update relationships

The runloop changes have the effect of making pushing relationships at least 50% faster, more if you have more relationships. It also fixes bugs like #2210

}
},

syncServer: function() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to flushCanonical

…ent relationship updates

This PR makes several changes to relationship handling:
1. It separates the path through which server side and client side updates happen, which lays the ground work for tracking relationship changes.
2. When pushing records into the store, it splits the push process into three phases using a custom runloop:
a) Push primary key data and attribtues
b) Turn foreign keys into records
c) Update relationships

The runloop changes have the effect of making pushing relationships at least 50% faster, more if you have more relationships. It also fixes bugs like #2210
@igorT igorT force-pushed the relationship_changes branch from 6640bfe to 947bd6a Compare December 18, 2014 01:14
@igorT igorT force-pushed the relationship_changes branch from 947bd6a to b1fdb4b Compare December 18, 2014 01:27
igorT added a commit that referenced this pull request Dec 18, 2014
@igorT igorT merged commit 2511ac8 into master Dec 18, 2014
@igorT igorT deleted the relationship_changes branch December 18, 2014 01:38
eccegordo added a commit to eccegordo/ed-dupe-bug that referenced this pull request Feb 5, 2015
Change ember data from beta 12 to beta 14.1

Steps to reproduce duplication bug.

1.) start server -> sane up
2.) navigate to http://localhost:4200/post
3.) Click "Add Comment" button couple of times
4.) Click "Delete" button next to each comment and remove all comments
5.) Click "Add Comment" button again.

Bug is that when generating a new comment record ember data will reproduce multiple instances of the record and render on screen. The behavior does not appear in ember data vs beta 12.

Bug seems to be caused by Ember Data pull request
emberjs/data#2576

Relationship changes #2576
igorT added a commit that referenced this pull request Mar 12, 2015
In #2576 we added a runloop inside ED,
in order to handle relationship deserializing and make sure we coalesce hasMany
/belongsTo changes. However in that PR we missed the findHasMany call, causing us
to trigger a flush(triggering an arrayWill/DidChange) once for every record being
parsed. This commit ensures we first process all the records together, and then call
flush only once. Was not sure how test this without a super specific unit test.
igorT added a commit that referenced this pull request Mar 12, 2015
In #2576 we added a runloop inside ED,
in order to handle relationship deserializing and make sure we coalesce hasMany
/belongsTo changes. However in that PR we missed the findHasMany call, causing us
to trigger a flush(triggering an arrayWill/DidChange) once for every record being
parsed. This commit ensures we first process all the records together, and then call
flush only once. Was not sure how test this without a super specific unit test.

fixes #2856
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.

2 participants