-
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
Splitting json adapter into two #958
Splitting json adapter into two #958
Conversation
Last tiny bit ...
We would need that one the be |
@spastorino done.
btw, as I mentioned there is no need to store the result for now. |
|
@spastorino 😢 tested over here: Can you share ur controller code?
|
ad3aa1b
to
eb5f189
Compare
eb5f189
to
e321cb3
Compare
Splitting json adapter into two
@@ -6,7 +6,7 @@ module Serialization | |||
|
|||
include ActionController::Renderers | |||
|
|||
ADAPTER_OPTION_KEYS = [:include, :fields, :root, :adapter] | |||
ADAPTER_OPTION_KEYS = [:include, :fields, :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.
This is a pretty big change. Prior to this, the root option always went to the adapter. Now it will always go to the serializer. Which code path should be dead? root to serializer or root to 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.
Yeah @bf4 this is indeed a big change.
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.
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'll have to read through the code some more to make sure I understand the changes as they affect tests that were deleted or whose expectations were changed. It didn't break #954 (once I rebased it), but I think this is the kind of thing we should include in the README between 'how it works' and 'intended usage'...
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.
Yeah, we realized we forgot about the README, but I'm already updating it.
I'm glad it didn't broke #954 😄
We discussed about the
root
option and how it shouldn't be a config but a different adapter itself.This PR create a
FlattenJson
adapter that doesn't supportroot
, theJson
adapter in the other hand will now haveroot
by default and you will still be able to manually set it with theroot
option.FlattenJson
is now the default adapter, the result still will be the same we had before, when the default wasJson
adapter without root.