Splitting json adapter into two #958

Merged
merged 14 commits into from Jun 16, 2015

Conversation

Projects
None yet
3 participants
@joaomdmoura
Member

joaomdmoura commented Jun 15, 2015

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 support root, the Json adapter in the other hand will now have root by default and you will still be able to manually set it with the root option.

FlattenJson is now the default adapter, the result still will be the same we had before, when the default was Json adapter without root.

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jun 15, 2015

I don't know the internals of the current solution but shouldn't you store this as it was done before?

I don't know the internals of the current solution but shouldn't you store this as it was done before?

This comment has been minimized.

Show comment
Hide comment
@joaomdmoura

joaomdmoura Jun 15, 2015

Owner

😅 Not sure what you meant by

store this as it was done before

Owner

joaomdmoura replied Jun 15, 2015

😅 Not sure what you meant by

store this as it was done before

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jun 15, 2015

Sorry :).

@result = { root => @result }

As long as I can see the whole result was being stored in @Result.
Exactly what is happening in Line 41 5932da6#diff-7b562c84aa219fb9dbfb8112b95df4d9L41

Sorry :).

@result = { root => @result }

As long as I can see the whole result was being stored in @Result.
Exactly what is happening in Line 41 5932da6#diff-7b562c84aa219fb9dbfb8112b95df4d9L41

This comment has been minimized.

Show comment
Hide comment
@joaomdmoura

joaomdmoura Jun 15, 2015

Owner

Oh got it.
So, there is particular need to store this right now because this is the last line / return from AMS.
We can store it if we want to, even without change the inheritance itself, by storing it into some other variable in both adapters, not sure about the name but something like serialized_object or serialized_return idk.

Owner

joaomdmoura replied Jun 15, 2015

Oh got it.
So, there is particular need to store this right now because this is the last line / return from AMS.
We can store it if we want to, even without change the inheritance itself, by storing it into some other variable in both adapters, not sure about the name but something like serialized_object or serialized_return idk.

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jun 15, 2015

In case you need to store you need to do the inheritance the other way around

In case you need to store you need to do the inheritance the other way around

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jun 15, 2015

In the long term I wouldn't even expose root, after all this it should be more like something internal to the format

In the long term I wouldn't even expose root, after all this it should be more like something internal to the format

This comment has been minimized.

Show comment
Hide comment
@joaomdmoura

joaomdmoura Jun 15, 2015

Owner

I'm only exposing to enable people to use a custom root if they want to.

Owner

joaomdmoura replied Jun 15, 2015

I'm only exposing to enable people to use a custom root if they want to.

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jun 15, 2015

I don't think that should happen. In my mind a format is defined without the possibility to change stuff, that's why we are providing this two things.

If someone wants to change something they should provide an adapter, inheriting if they want to take advantage of something.

I don't think that should happen. In my mind a format is defined without the possibility to change stuff, that's why we are providing this two things.

If someone wants to change something they should provide an adapter, inheriting if they want to take advantage of something.

This comment has been minimized.

Show comment
Hide comment
@joaomdmoura

joaomdmoura Jun 15, 2015

Owner

I got it and agree, but "deprecating" it now might make it harder to ppl willing to update to AMS 0.10.x.
What is your thought about it? Should we take advantage that we are on a RC and remove it, or wait for it?

Owner

joaomdmoura replied Jun 15, 2015

I got it and agree, but "deprecating" it now might make it harder to ppl willing to update to AMS 0.10.x.
What is your thought about it? Should we take advantage that we are on a RC and remove it, or wait for it?

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jun 15, 2015

Not sure to be honest, according to what I see the migration is already a bit hard so I'm not sure if the root things changes anything.

Not sure to be honest, according to what I see the migration is already a bit hard so I'm not sure if the root things changes anything.

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jun 15, 2015

Member

Last tiny bit ...

curl http://localhost:3000/todos
{"todo":[{"id":1,"title":"Todo 1","completed":false,"order":1},{"id":3,"title":"Todo 8","completed":true,"order":2}]}

We would need that one the be todos as it was in old AMS, that's the way it plays nice with Ember's Rest adapter.

Member

spastorino commented Jun 15, 2015

Last tiny bit ...

curl http://localhost:3000/todos
{"todo":[{"id":1,"title":"Todo 1","completed":false,"order":1},{"id":3,"title":"Todo 8","completed":true,"order":2}]}

We would need that one the be todos as it was in old AMS, that's the way it plays nice with Ember's Rest adapter.

@joaomdmoura

This comment has been minimized.

Show comment
Hide comment
@joaomdmoura

joaomdmoura Jun 15, 2015

Member

@spastorino done.

  • Removed root option (removed the tests related to it)
  • Pluralizing root when using ArraySerializer

btw, as I mentioned there is no need to store the result for now.

Member

joaomdmoura commented Jun 15, 2015

@spastorino done.

  • Removed root option (removed the tests related to it)
  • Pluralizing root when using ArraySerializer

btw, as I mentioned there is no need to store the result for now.

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jun 15, 2015

Member
curl http://localhost:3000/todos
{"todo":[{"id":1,"title":"Todo 1","isCompleted":null}]}
Member

spastorino commented Jun 15, 2015

curl http://localhost:3000/todos
{"todo":[{"id":1,"title":"Todo 1","isCompleted":null}]}
@joaomdmoura

This comment has been minimized.

Show comment
Hide comment
@joaomdmoura

joaomdmoura Jun 15, 2015

Member

@spastorino 😢 tested over here:
(you can check the code here)

Can you share ur controller code?
Did you defined ActiveModel::Serializer.config.adapter = :json?
Is it pointed to the last commit?

curl http://localhost:3000

{ 
  "posts": [
    {"id":1,"title":"Awesome Post","body":"Indeed a great one!","user":{"id":2,"name":"Mattew Crazy","age":33,"email":"mattew@crazy.com"}},
    {"id":2,"title":"Incredible Post","body":"Indeed another great one!","user":{"id":2,"name":"Mattew Crazy","age":33,"email":"mattew@crazy.com"}}
  ]
}
Member

joaomdmoura commented Jun 15, 2015

@spastorino 😢 tested over here:
(you can check the code here)

Can you share ur controller code?
Did you defined ActiveModel::Serializer.config.adapter = :json?
Is it pointed to the last commit?

curl http://localhost:3000

{ 
  "posts": [
    {"id":1,"title":"Awesome Post","body":"Indeed a great one!","user":{"id":2,"name":"Mattew Crazy","age":33,"email":"mattew@crazy.com"}},
    {"id":2,"title":"Incredible Post","body":"Indeed another great one!","user":{"id":2,"name":"Mattew Crazy","age":33,"email":"mattew@crazy.com"}}
  ]
}

joaomdmoura added a commit that referenced this pull request Jun 16, 2015

@joaomdmoura joaomdmoura merged commit bc8fd0a into rails-api:master Jun 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -6,7 +6,7 @@ module Serialization
include ActionController::Renderers
- ADAPTER_OPTION_KEYS = [:include, :fields, :root, :adapter]
+ ADAPTER_OPTION_KEYS = [:include, :fields, :adapter]

This comment has been minimized.

@bf4

bf4 Jun 16, 2015

Member

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?

@bf4

bf4 Jun 16, 2015

Member

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?

This comment has been minimized.

@joaomdmoura

joaomdmoura Jun 16, 2015

Member

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.

@joaomdmoura

joaomdmoura Jun 16, 2015

Member

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.

This comment has been minimized.

@bf4

bf4 Jun 16, 2015

Member

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'...

@bf4

bf4 Jun 16, 2015

Member

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'...

This comment has been minimized.

@joaomdmoura

joaomdmoura Jun 16, 2015

Member

Yeah, we realized we forgot about the README, but I'm already updating it.
I'm glad it didn't broke #954 😄

@joaomdmoura

joaomdmoura Jun 16, 2015

Member

Yeah, we realized we forgot about the README, but I'm already updating it.
I'm glad it didn't broke #954 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment