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

Note change in where the root goes, and flatten json #966

Closed
wants to merge 1 commit into from

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Jun 24, 2015

This is for discussion. I intend to improve the docs and probably
write some code and tests, as well.

Extracted from #954

@joaomdmoura
Copy link
Member

@bf4 didn't understood the proposal of this PR 😅
I have updated the README with some FlattenJSON superficial info.
I'll push the wiki thing forward tonight, then we will be able to concentrate all specific and more technical info in one place 😄

Which code path should be dead? root to serializer or root to adapter?

R. I'm not sure but root to serializer seems good to me.

@bf4
Copy link
Member Author

bf4 commented Jun 26, 2015

Ok, I just wanted to be clear that previously, in 0.10 both the adapters and serializers accepted a root option, but, via the controller, only the adapter ever got it. Since #958, via the controller, only the serializer ever gets the root option. I don't think having two places that accept root, but only one is ever used, is a good thing. So, if I understand your comment there

The idea is that root shouldn't be an option, but another adapter by itself.
This make it even easier to integrate with some js frameworks.
We also removed the option to set a specific root key, so the root isn't "going" to the serializer, but is the serializer the one that knows what it should be and the adapter the one the uses it or not.

  • The root option no longer means the root in the json response, since that was always really the resource root, and the serializer should determine the resource root.
  • a serialized resource defaults the root to the json_key, derived from the class name, if none is given
  • Having a root in the fully serialized response is a function of the adapter, such that the json-api adapter might use 'data', the json adapter might use the root from the serialized resource, and the flatten adapter ignores the root entirely.

@joaomdmoura
Copy link
Member

@bf4 thank you for quoting this here 😄

The root option no longer means the root in the json response, since that was always really the resource root, and the serializer should determine the resource root.

Yeah, root option no longer means the root in json response. The serialize determinate he resource root.

a serialized resource defaults the root to the json_key, derived from the class name, if none is given

The serializer always determinate the root based on class name, there is no way to override it.

Having a root in the fully serialized response is a function of the adapter, such that the json-api adapter might use 'data', the json adapter might use the root from the serialized resource, and the flatten adapter ignores the root entirely.

Exactly, perfect comparison using JSON API.

@joaomdmoura
Copy link
Member

This is for discussion.

can we close this @bf4?
The actual README has an description of the new behaviour already.

FlattenJSON

It's the default adapter, it generates a json response without a root key. Doesn't follow any specifc convention.

JSON

It also generates a json response but always with a root key. The root key can't be overridden, and will be automatically defined accordingly with the objects being serialized. Doesn't follow any specifc convention.

@bf4
Copy link
Member Author

bf4 commented Jul 7, 2015

@joaomdmoura Depends on how you want to resolve #987

@joaomdmoura
Copy link
Member

@bf4, just merged #1013

@bf4
Copy link
Member Author

bf4 commented Jul 17, 2015

Yeah, I kind of don't like the implementation there, but I guess I can rewrite this as a PR to docs to clarify what the root option affects

@joaomdmoura
Copy link
Member

Agreed! @bf4 I'm closing this one in favor of a new PR implementing this on the new Docs 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.

2 participants