Improve current-user service guides #1139
Conversation
|
@pangratz: can you review? |
|
|
||
| Using the `queryRecord` method and overriding the adapter avoid this problem. | ||
| The adapter override will generate `api/users/me` when the `me` query param is | ||
| present. |
There was a problem hiding this comment.
Let's change the ending to something like this:
... when store.queryRecord() is invoked with a query where the me param is present.
| }); | ||
| let userId = this.get('session.data.authenticated.user_id'); | ||
| if (!isEmpty(userId)) { | ||
| return this.get('store').find('user', userId).then((user) => { |
|
|
||
| If the `find` method is called with id `me` and the response includes the actual | ||
| user id, ember data will create an empty record with id `me` and all other model | ||
| attributes empty. |
There was a problem hiding this comment.
find is actually private, I think we should use store.findRecord here as it is more idiomatic ember-data use, even though it has been mentioned before.
I am not sure we should state here why findRecord('user', 'me') is the wrong choice and instead say why we are using store.queryRecord(): because it's the way to fetch a record in ember data when the id is not known beforehand.
And then maybe outline with a Note: why we are not using findRecord("user", "me"): because ember data expects the returned model to have the same id, as otherwise an unused empty record with id of me is in the store.
There was a problem hiding this comment.
Thanks for the feedback, I'll reword it :)
|
@juanazam overall this looks great! Thanks so much for tackling this! |
juanazam
force-pushed
the
improve_current_user_guides
branch
from
cdb86cc to
81bd8f6
Compare
|
@pangratz thanks for reviewing and commenting! I have updated my changes with your suggestions, hope it looks ok now. |
| @@ -64,14 +64,57 @@ export default Ember.Service.extend({ | |||
|
|
|||
| load() { | |||
| if (this.get('session.isAuthenticated')) { | |||
| return this.get('store').find('user', 'me').then((user) => { | |||
| return this.get('store').findRecord('user', 'me').then((user) => { | |||
There was a problem hiding this comment.
We should probably have queryRecord and make what is described in "Using a dedicated endpoint and returning actual user id in response" below the default recommendation as findRecord will lead to problems and thus shouldn't be in the guides at all.
There was a problem hiding this comment.
So, if I understood correctly, we should remove the using a dedicated endpoint section right? or use that title with the code used in the queryParams example.
There was a problem hiding this comment.
Yeah, replace this sample with the one using queryRecord you added.
| unused empty record with `id` of `me` is in the store. | ||
|
|
||
| The adapter override will generate `api/users/me` when `store.queryRecord` is | ||
| invoked with a query param where the `me` param is present. |
There was a problem hiding this comment.
We should put the explanation before the code sample so the guide is easier to follow when reading through it top-down.
juanazam
force-pushed
the
improve_current_user_guides
branch
from
81bd8f6 to
c6aafe2
Compare
|
@marcoow thanks for your suggestions, I have included those changes too. |
|
🎉 |
| let userId = this.get('session.data.authenticated.user_id'); | ||
| if (!isEmpty(userId)) { | ||
| return this.get('store').findRecord('user', userId).then((user) => { | ||
| this.set('user', user); |
There was a problem hiding this comment.
Shouldn't there be another return Ember.RSVP.resolve(); here?
This PR aims to improve
current-userservice guides a bit, including the case when using a dedicated endpoint but including the actual current using id in the response instead of a special value (e.g.: me).The newly added section explains why using EmberData's
findmethod with idmeand returning an actual id in the response is a problem. For more information, please check: emberjs/data#4414.It also closes a stalled issue and PR:
#1064
#1065