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

Merge normalizeRelationships and setupRelationships methods in store #3854

Closed
wants to merge 1 commit into from

Conversation

trentmwillis
Copy link
Member

While working on some performance related tasks, I noticed that normalizeRelationships and setupRelationships both loop over the relationships for a record. The only time these functions are used is in sequence, so it looks like these could be merged to cut down on some time complexity.

Will be trying this out in our application tomorrow to see if it has any side effects, but it doesn't appear to so far.

@tchak
Copy link
Member

tchak commented Oct 15, 2015

LGTM 👍

@trentmwillis
Copy link
Member Author

Any chance this could get merged? Just updated to remove the type param (not sure it was needed to begin with).

@wecc
Copy link
Contributor

wecc commented Oct 22, 2015

@igorT r?

@@ -2135,13 +2108,16 @@ function setupRelationships(store, record, data) {
relationship = record._relationships.get(key);
relationship.updateMeta(data.relationships[key].meta);
}

var value = data.relationships[key].data;
Copy link
Member

Choose a reason for hiding this comment

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

can we normalize here, and put the comment from 1727 back in?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can, but not sure I see the point in doing that. If we normalize here, then we will have to check that value exists and the kind of relationship in two places (here and then again below).

Copy link
Member

Choose a reason for hiding this comment

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

Mostly for code readability/place to put the deleted comment/ease of
understanding
On Oct 22, 2015 10:04, "Trent Willis" [email protected] wrote:

In packages/ember-data/lib/system/store.js
#3854 (comment):

@@ -2135,13 +2108,16 @@ function setupRelationships(store, record, data) {
relationship = record._relationships.get(key);
relationship.updateMeta(data.relationships[key].meta);
}
+
var value = data.relationships[key].data;

I can, but not sure I see the point in doing that. If we normalize here,
then we will have to check that value exists and the kind of relationship
in two places (here and then again below).


Reply to this email directly or view it on GitHub
https://github.com/emberjs/data/pull/3854/files#r42775635.

@igorT
Copy link
Member

igorT commented Oct 22, 2015

👍 after the small refactor. Thanks for the PR

@trentmwillis
Copy link
Member Author

@igorT updated. Let me know if there's anything else.

@bmac
Copy link
Member

bmac commented Dec 3, 2015

Thanks @trentmwillis. I have rebased this pr and merged is as 828f8ea.

@bmac bmac closed this Dec 3, 2015
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.

5 participants