-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Update docs for JSONAPI relationship links #1935
base: master
Are you sure you want to change the base?
Conversation
@ksamc, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vasilakisfil to be a potential reviewer |
Interesting! I didn't know this method (#1710) at all and I was fixing this issue from the ember's side in a serializer. Looks good to me! |
@@ -37,7 +37,7 @@ class Api::V1::UserSerializer < ActiveModel::Serializer | |||
end | |||
``` | |||
|
|||
This will resilt in (example is in jsonapi adapter): | |||
This will result in (example is in jsonapi adapter): |
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
@@ -69,7 +69,7 @@ class Api::V1::UserSerializer < ActiveModel::Serializer | |||
end | |||
``` | |||
|
|||
This will resilt in (example is in jsonapi adapter): | |||
This will result in (example is in jsonapi adapter): |
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
#more on: https://github.com/rails-api/active_model_serializers/issues/1325 | ||
def microposts | ||
object.microposts.loaded ? object.microposts : object.microposts.none | ||
include_data false |
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.
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.
For this specific comment, I think it is no longer needed. include_data false
is already documented elsewhere as avoiding the N+1/loading the data.
That said, we should probably rework this section. It's totally valid to have both data
and links
, so there's really no inherent reason to have include_data false
in the documentation example in the first place. We're just talking about link generation here.
Instead, I'd add a section below this (or elsewhere) on conditionally including data
, how and why one would do this. @ksamc any interest in adding this documentation, based on the explanation in #1931? Otherwise I will submit a separate PR.
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.
@richmolj I would say the opposite is more often: if you add links you probably don't include the relationship.
I would suggest to merge this request and you can start a new one about conditionally adding data (probably in another section of the wiki)
Please commit these changes because I almost losed my mind. |
Purpose
Fixes small typos and updated the docs on relationship links for JSONAPI.
Changes
Removed the 'data' key in the serialized relationship. Including it according to the current docs leads to unexpected behavior when using it with libraries like Ember Data.
This method returns an empty array if the relationship is not loaded even though they might exist. Passing an empty array as data tells clients that there are no existing relationships.
Instead data should not be sent at all so that the client uses the given nested resource link to fetch them.
Caveats
Related GitHub issues
Additional helpful information