-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implement basic JSON-API adapter #2904
Conversation
|
||
pathForType: function(typeKey) { | ||
return Ember.String.dasherize(typeKey); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
eb34c6d
to
57bc19c
Compare
return this._super(attributeType, true) || transforms[attributeType]; | ||
}; | ||
DS.JSONSerializer.reopen({ transformFor: transformFor }); | ||
DS.JSONAPISerializer.reopen({ transformFor: transformFor }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like transformFor
is never defined on the serializer and adapter, but the tests pass because of this patching (would not work in an actual app). I think?
610a430
to
aa31e23
Compare
@namespace DS | ||
@extends DS.Serializer | ||
*/ | ||
export default Serializer.extend({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this extend from JSONSerializer
so it can inherit the transformFor
method?
hi, this is still in the RC3 format and not the final specification of jsonapi 1.0 right? Are you planning to finish this for 1.0? Can I help out in any way? Please give a small status update, thanks :) |
@wwwdata I'm hard at work updating this to both 1.0 and the new Serializer API refactor we did recently. I hope I can update this PR later tonight with the new code! Happy to have an extra set of 👀 reviewing it! |
@wecc awesome! I will review it carefully and test it against my backend once you push the new commits |
@wwwdata ping |
export default RESTAdapter.extend({ | ||
defaultSerializer: '-json-api', | ||
|
||
concatenatedProperties: ['headers'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are gonna do this, should add it to the rest adapter, but it seems like they can also be a CP as per the comments in the rest adapter, so might not work
} | ||
|
||
let data = hasMany.map((item) => { | ||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can rewrite without return and {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forEach and data.push()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs EnumerableUtils for IE8.
👍 |
I can't express the amount of awesome there is in the ember community! Thanks for all the amazing work. 👍 |
|
||
run(function() { | ||
store.find('post').then(function(posts) { | ||
equal(passedUrl[0], '/post'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the url should be /posts here. They are always pluralized.
|
||
run(function() { | ||
store.find('post', 1).then(function(post) { | ||
equal(passedUrl[0], '/post/1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wwwdata same here as down below: this should be /posts/1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so @wecc it seems that most/all URLs in the tests are singular at the moment ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently intentional as JSON-API does not state anything about URLs being singular/plural. Right now we only pass through from whatever your model names are. If your model is named Post
the URL will be /post/1
. If it's named Posts
the URL will be /posts/1
.
I'm very open to give Ember Data stronger opinions on this. Like plural for URLs and types and dasherized keys in payloads to match the examples from http://jsonapi.org/. There simply needs to be a decision made. I will bring this up at our meeting later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are completely right @wecc. Thanks for the heads up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, but it is very confusing to name a single model in plural like posts
. If we do not want to enforce a pluralized url we need some mechanism to configure the url I guess. On the jsonapi.org website, all the URLs are plural. A lot of backends that generate an API with URLs will generate them in that recommended format, so it should be easy for an ember-data user to use this recommended url scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree with the confusion. For me:
- Url should be plural:
/api/posts
or/api/posts/5
(REST compliant) - Model should be singular:
Post
- the
type
key in a jsonapi payload can be singular or plural (according to the spec). In my opinion, we need an option in the JSONAPIAdapter to specify the singular or plural form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pangratz @wwwdata @SeyZ this PR has now been updated to match the format on jsonapi.org as closely as possible.
- types are serialized to, and expected to be plural dasherized when received from the server
- member names are serialized to, and expected to be dasherized when received from the server
However, the internal JSON-API format for ED has singular dasherized for types and camelized for member names. This should not be an issue as the JSONAPISerializer
normalizes the format mentioned above to the internal ED format automatically.
All of these will be easily overridden using hooks (modelNameFromPayloadKey
, payloadKeyFromModelName
, extractType
, keyForAttribute
, keyForRelationship
...)
URLs are now also built to be plural dasherized by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍 awesome work @wecc!
let hash = this._super(...arguments); | ||
|
||
if (hash.contentType) { | ||
hash.contentType = 'application/vnd.api+json; charset=utf-8'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think parameters, like charset=utf-8
, are allowed by the spec right now. See content negotiation. (Same thing on line 33.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
00f6206
to
5d59b76
Compare
}, | ||
|
||
/** | ||
TODO: Remove this once we have a better way to override HTTP verbs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets move this comment out of the doc block.
*/ | ||
_normalizeResponse: function(store, primaryModelClass, payload, id, requestType, isSingle) { | ||
|
||
let normalizeResourceHelper = (resourceHash) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably move this to the prototype to avoid re-creating a closure every time.
Overall looks pretty good. You, bmac, and igor did all the hard work which is why this is a relatively small change 💃 |
@param {Object} resourceHash | ||
@return {String} | ||
*/ | ||
extractType: function(modelClass, resourceHash) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
underscore
Implement basic JSON-API adapter
this is merged now, which is awesome but did anybody actually use this adapter against his backend and can help me? When I want to load entities from my backend, the request is made, I can see that in chrome debug tools but I always have an empty array instead of a list of my I am doing this.store.findAll('question'); in my route question model import DS from 'ember-data';
export default DS.Model.extend({
text: DS.attr('string')
}); the server returns this very minimalistic json {
"data": [
{
"attributes": {
"text": "wow?"
},
"id": "ddb72a88-3004-4c30-a14c-71f50084233b",
"type": "questions"
}
]
} I am using ember-cli and registered the JSONAPIAdapter in my application adapter and the JSONAPISerializer as application serializer. What could I have possibly done wrong that this does not work? |
@wwwdata I'm able to replicate that the JSONSerializer doesn't automatically use the new Serializer API when extending. Can you confirm if it works when you change your application serializer to: export default DS.JSONAPISerializer.extend({
isNewSerializerAPI: true
}); |
aaah, thank you so much, yes that was the problem! |
A basic working JSON-API adapter.