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

[CLEANUP beta] Change the way metadata in response is stored on record array #4077

Merged
merged 1 commit into from
Jan 15, 2016
Merged

[CLEANUP beta] Change the way metadata in response is stored on record array #4077

merged 1 commit into from
Jan 15, 2016

Conversation

danmcclain
Copy link
Contributor

The previous implementation of inserting metadata from an ajax response
onto a record array used the store as temporary storage for the metadata
via store._metadataFor and store._setMetadataFor. Although rare, this could
store the wrong value on the record array due to a race condition, as
the metadata was stored based on the query's model's name. This moves
the storing of metadata to AdapterPopulatedRecordArray.loadRecords
since, at the time that that method is called, the payload is present,
so we don't have to use store.{_setMetadataFor,_metadataFor}.
store._metadataFor was only used in
AdapterPopulatedRecordArray.loadRecords, so it is no longer needed
with this new implementation.

@bmac
Copy link
Member

bmac commented Jan 14, 2016

Looks good to me. @wecc can your also review this pr?

@danmcclain
Copy link
Contributor Author

I am planning on opening an additional PR to add links to the record array like meta, do you want me to bring it into this PR? Or keep it separate?

@bmac
Copy link
Member

bmac commented Jan 14, 2016

I'd say do it in a separate pr.

@danmcclain
Copy link
Contributor Author

Ok, I'll wait for this PR to be accepted first, as it will tie into the same loadRecords function 👍

@wecc
Copy link
Contributor

wecc commented Jan 14, 2016

Thanks for the PR @danmcclain!

It's nice to see _metadataFor and _setMetadataFor finally go :) I'm trying to remember why we didn't do this for the 1.13 release/New Serializer API refactor but I'm struggling. I think it was along the lines that we didn't want both records and the payload to "leak" to the array. I'm inclined to suggest to just put meta on the array like we do here but I can see how that might make continued JSON API compliance more verbose.

I'm not gonna block this due to ⬆️, the code is private and easily changed if needed. It's very nice to get rid of _metadataFor and _setMetadataFor acting like a proxy, I'm 👍

Can you prefix the commit with [CLEANUP beta]? (not really a BUGFIX imo)

…d array

The previous implementation of inserting metadata from an ajax response
onto a record array used the store as temporary storage for the metadata
via `store._metadataFor` and `store._setMetadataFor`. Although rare, this could
store the wrong value on the record array due to a race condition, as
the metadata was stored based on the query's model's name. This moves
the storing of metadata to `AdapterPopulatedRecordArray.loadRecords`
since, at the time that that method is called, the payload is present,
so we don't have to use `store.{_setMetadataFor,_metadataFor}`.
`store._metadataFor` was only used in
`AdapterPopulatedRecordArray.loadRecords`, so it is no longer needed
with this new implementation.
@danmcclain danmcclain changed the title Change the way metadata in response is stored on record array [CLEANUP beta] Change the way metadata in response is stored on record array Jan 14, 2016
@danmcclain
Copy link
Contributor Author

@wecc Updated!

I was thinking the same (just passing payload.meta), but I am actually adding links support on one of our projects and grabbed links off of payload as well. When I open the links PR, I'll discuss the approach I've taken for our project

Thanks

@danmcclain
Copy link
Contributor Author

I take that back, I never pushed it until now. Updated for real @wecc

bmac added a commit that referenced this pull request Jan 15, 2016
[CLEANUP beta] Change the way metadata in response is stored on record array
@bmac bmac merged commit a6b7716 into emberjs:master Jan 15, 2016
@danmcclain danmcclain deleted the eliminate-metadataFor branch January 15, 2016 15:11
@amk221
Copy link
Contributor

amk221 commented Mar 4, 2016

I was using @vanadar's temporary solution to get meta for a single record. Since this PR it no longer works. Is there an alternative?

@duizendnegen
Copy link

Similar issue, the workaround in #4115 now no longer works.

@amk221
Copy link
Contributor

amk221 commented Mar 24, 2016

@duizendnegen hack:

// models/base.js
_meta: computed(function() {
  return this._internalModel.type.____meta;
}).volatile()
// serializers/application.js
extractMeta(store, typeClass) {
  let meta = this._super(...arguments);
  typeClass.____meta = meta;
  return meta;
},
// usage
myModel.get('_meta');

kevinansfield added a commit to naz/Ghost-Admin that referenced this pull request Dec 17, 2019
no issue

- Ember Data does not support accessing meta data included in the response to single-record requests such as save/delete
- approach to allow it taken from emberjs/data#4077 (comment)
naz pushed a commit to naz/Ghost-Admin that referenced this pull request Dec 18, 2019
no issue

- Ember Data does not support accessing meta data included in the response to single-record requests such as save/delete
- approach to allow it taken from emberjs/data#4077 (comment)
kevinansfield added a commit to naz/Ghost-Admin that referenced this pull request Feb 6, 2020
no issue

- Ember Data does not support accessing meta data included in the response to single-record requests such as save/delete
- approach to allow it taken from emberjs/data#4077 (comment)
daniellockyer pushed a commit to TryGhost/Admin that referenced this pull request Feb 10, 2020
no issue

- Ember Data does not support accessing meta data included in the response to single-record requests such as save/delete
- approach to allow it taken from emberjs/data#4077 (comment)
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