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

Has many metadata #2878

Closed
wants to merge 2 commits into from
Closed

Has many metadata #2878

wants to merge 2 commits into from

Conversation

wecc
Copy link
Contributor

@wecc wecc commented Mar 14, 2015

No description provided.

@wecc wecc force-pushed the has-many-metadata branch 3 times, most recently from 2680704 to 615f58a Compare March 15, 2015 22:27
@bmac bmac added this to the 1.0.0-beta.16 milestone Mar 17, 2015
@wecc wecc mentioned this pull request Mar 19, 2015
10 tasks
@wecc
Copy link
Contributor Author

wecc commented Mar 21, 2015

This should be good to merge.

@@ -0,0 +1,9 @@
function cloneNull(source) {
Copy link
Member

Choose a reason for hiding this comment

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

export default function cloneNull

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected, thanks!

@bmac bmac removed this from the 1.0.0-beta.16 milestone Mar 22, 2015
@@ -144,6 +145,9 @@ ManyRelationship.prototype.computeChanges = function(records) {
ManyRelationship.prototype.fetchLink = function() {
var self = this;
return this.store.findHasMany(this.record, this.link, this.relationshipMeta).then(function(records) {
var meta = self.store.metadataFor(self.relationshipMeta.type);
self.manyArray.set('meta', cloneNull(meta));
Copy link
Member

Choose a reason for hiding this comment

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

We have a potential problem here, if you make a request for a record, set some metadata, and then fetch the link, do not set the metadata, this array will end up with wrong metadata. Not sure how to fix this right now

@wecc wecc force-pushed the has-many-metadata branch 2 times, most recently from 2904134 to c71260d Compare April 13, 2015 09:03
@wecc
Copy link
Contributor Author

wecc commented Apr 13, 2015

@igorT Rebased and merge conflicts fixed, should be good to go. Moved failing test to #2996.

@igorT
Copy link
Member

igorT commented Jun 6, 2015

image
such lies

igorT added a commit that referenced this pull request Jun 6, 2015
Rebased #2878 and added a test from #2996
Added metadata support behind a feature flag, for hasMany relationships.
For now, until we refactor the internals, the store internally pushes hasMany
metadata around in the `meta` key next to `links` and `data`.
This might lead to overriding meta attribtues with meta relationships,
but will be solved by the upcoming change of the internals to JSONAPI format
@igorT igorT mentioned this pull request Jun 6, 2015
@igorT
Copy link
Member

igorT commented Jun 6, 2015

Superseded by #3221

@igorT igorT closed this Jun 6, 2015
igorT added a commit that referenced this pull request Jun 9, 2015
Rebased #2878 and added a test from #2996
Added metadata support behind a feature flag, for hasMany relationships.
For now, until we refactor the internals, the store internally pushes hasMany
metadata around in the `meta` key next to `links` and `data`.
This might lead to overriding meta attribtues with meta relationships,
but will be solved by the upcoming change of the internals to JSONAPI format
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.

4 participants