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

Serialization/Deserialization Requirements & Implementation #96

Open
ajjahn opened this issue Dec 9, 2015 · 9 comments
Open

Serialization/Deserialization Requirements & Implementation #96

ajjahn opened this issue Dec 9, 2015 · 9 comments
Milestone

Comments

@ajjahn
Copy link
Collaborator

ajjahn commented Dec 9, 2015

Since I've been harping on this subject and recently some related issues (#94, #95) have been opened, it seems like a good time to get a discussion regarding where we'd like to see serialization for passing objects into the pre-rendering system to go and how we'd like it to work.

Here are my initial thoughts:

  • Core classes in either the Ruby or Opal runtimes should not be opened up and extended with additional methods and behavior. This means removing react_serializer and _react_param_conversion. I believe those to be an anti-pattern. We'd be much better suited using composition than extending the responsibilities of classes. A hash shouldn't care about the details of how it converts to a transport format and back. Building simple classes like, HashSerializer, ArraySerializer, etc, would separate concerns, and allow for serializers to be swapped out with custom objects. MyFancyHashThingSerializer for example.
  • Core classes should be serialized and deserialized by default.
  • Custom objects can provide custom serialization/deserialization objects. BlogPostSerializer for example.
  • Maybe provide something like a SimpleObjectSerializer that can be instructed which methods/attributes of a custom class should be serialized and deserialized without needing to define a custom serializer.

Question: What are some other scenarios and uses cases we should be taking into account?

@ajjahn ajjahn added this to the 0.8.x milestone Dec 21, 2015
@catmando
Copy link
Collaborator

@ajjahn - I think I understand how this would work:

  1. naming convention would look for a class (module?) called <class_name>ReactSerializer, and if such a class (module?) existed would use use its self.serialize/deserialize methods.

So we would add HashReactSerializer, etc to react.rb (Hash may be the only one needed actually)

And ReactiveRecord would add ActiveRecordReactSerializer...

okay so far?

Assuming so, then I have two questions:

  1. is the lookup for the matching serializer done by searching the classes inheritence tree, so if you have class Foo < Hash then you would first look for FooReactSerializer, the HashReactSerializer, and finally ObjectReactSerializer ??? Or does ActiveRecord::Base have to automagically define serializer classes as each time ActiveRecord::Base is subclassed?

  2. how this all allow a different Serializer to be swapped in, as you suggest?

@ajjahn
Copy link
Collaborator Author

ajjahn commented Mar 15, 2016

I'm going to stream of conscience pseudo code this, so it doesn't have to look exactly like this.

We provide the simplest serializer. It simply relies on ActiveSupport which will call to_json on the value:

class BaseSerializer
  def initialize(value)
    @value = value
  end

  def to_json
    # ActiveSupport already knows how to handle built in collections and value objects.
    ActiveSupport::JSON.encode(@value)
  end

  # Could be handy to make the serializer callable.
  def call
    to_json
  end
end

Let's say we have a plain old ruby object we'd like to serialize:

class Car
  attr_accessible :make, :year
end

class CarSerializer < BaseSerializer
  def to_json
    { make: make, year: year }.to_json # or whatever custom serialize logic you want.
  end
end

Pretty much the same for ActiveRecord models:

class Employee
  belongs_to :company
end

class EmployeeSerializer < BaseSerializer
  def as_json
    @value.as_json(only: [:name, :email], include: [:company])
  end
end

  1. is the lookup for the matching serializer done by searching the classes inheritence tree, so if you have class Foo < Hash then you would first look for FooReactSerializer, the HashReactSerializer, and finally ObjectReactSerializer ???

When we render :component in a controller or view, we can select the correct serializer based on the class of the passed in object. No need to traverse trees, just stick to a convention.

# somewhere in the render helper
serializer = begin
  "#{object.class.name}Serializer".constantize
rescue
  BaseSerializer
end

# Serialize it
serializer.new(object).to_json

  1. how this all allow a different Serializer to be swapped in, as you suggest

Pass an explicit serializer as an option to render :component

render component: Employee, employee: @employee, serializer: EmployeeWithTitleSerializer

Or does ActiveRecord::Base have to automagically define serializer classes as each time ActiveRecord::Base is subclassed?

It doesn't really make sense to have an 'ActiveRecord' serializer. Data serialization is a domain concern. If you wanted a serializer that serialized all the attributes of an ActiveModel object, the base serializer would do the trick since as_json does that be default.


After typing all this out... I don't see any reason we can't just lean on something like ActiveModelSerializers outright. It already solves this problem for us, not to mention it is part of JSON-API, which is now merged in to Rails 5.

We probably should implement react.rb's render helper as a true ActionController::Renderer. I'm sure we could easily offload the serialization to ActiveModel::SerializableResource.new(resource, options).to_json

Anyway, does that help illustrate where I was going with this?

@catmando
Copy link
Collaborator

Yes the basics of what you outlined were exactly what I was figuring. My questions still stand however.

RE: "When we render :component in a controller or view, we can select the correct serializer based on the class of the passed in object. No need to traverse trees, just stick to a convention."

But that means that if I design a serializier for a base class (Foo) and then have a subclass, I don't automatically get the serialization behavior. That is why I thought we would have to traverse down the class hierarchy.

RE: render component: Employee, employee: @employee, serializer: EmployeeWithTitleSerializer

And if you have two objects being passed? How does it know which object that serializer goes with?

RE: It doesn't really make sense to have an 'ActiveRecord' serializer. Data serialization is a domain concern. If you wanted a serializer that serialized all the attributes of an ActiveModel object, the base serializer would do the trick since as_json does that be default.

This is the exact problem that got us into this... people do override as_json for whatever purpose. If we could count on "as_json" working, then we don't need any of this, and could just call the objects as_json method...

Another Side Topic Related

Right now we are determining the "type" based on the optional "type" of the param macro.

This has already caused several new users to stumble, as its optional, but not really if you are passing data from the server, that needs serialization.

Perhaps we should pass the serializer class name as part of the top level params. In otherwords the render method would call the serializer, and then as part of the params to the "TopLevelComponent" would pass the list of serializer classes used.

The TopLevelComponent would then do the deserialization before calling the actual component.

@wied03
Copy link
Contributor

wied03 commented Mar 22, 2016

I'm sort of re-emerging here so forgive the lack of context, but is this meant to be done in a "reactive record" gem or in the (simple) Opal wrapper around React? I would certainly say serialization should not be a part of the basic GEM.

Putting aside which GEM it belongs in, for my application, I've just done a fairly simple setup where I whitelist a few serializable attributes for a class and then override #as_json to serialize those attributes on the server side. I then have a ::json_create method that I've overloaded on the Opal side.

React (on opal) never has to know about any of this except for 1 base 'Store' component that deserializes the JSON (which is my initial state) using JSON.load and instantiates a GrandCentral store.

There is no serialization back to the server in my case (wouldn't want to anyways due to security issues with deserializing on my machine).

@catmando
Copy link
Collaborator

Its not just applicable to reactive-record (although reactive-record certainly needs some mechanism)

The intention here is to provide some standard mechanism so that things "just work" in an obvious way, and that gems like reactive-record can use it as well.

When I say obvious way, I mean if I pass some structure as a param from the server, I expect it to appear reconstituted on the client (assuming the class is isomorphic.) So I do think there needs to be some built-in mechanism to take care of the deserialization.

The more I think about it, the better I like the idea of making this the responsibility of TopLevelComponent. This component is part of the rails interface and I think we all agree all the rails adapter code should go in its own gem. That will keep this whole issue out of the react.rb code, it will also make it independent of the param "type" declaration (something I know @wied03 wants to get away from.)

However that leaves us with the issue of how to declare the serializer/deserializers. The code currently is basically as @wied03 describes it above except for the method names.

@ajjahn proposed using separate serialization classes, which seems great, but there were a few pending questions above.

@wied03
Copy link
Contributor

wied03 commented Mar 22, 2016

I should add that I never send ActiveRecord models to the client. In my view they are too mutable and complex to work well with the React/Redux/GrandCentral architecture. I build "view models" and those are what I serialize with Rails and deserialize with the approach I noted above.

@catmando
Copy link
Collaborator

So this is blocking cleaning up PropsWrapper which is blocking context support, so it would be good to wrap this up.

I think the best approach is to get this out of the base react.rb param mechanism, and move it to the TopLevelComponent as mentioned above. This decouples the whole mess from react.rb, makes it so you don't have to "type" params, and makes everything work much more naturally.

Again that proposal is to have anything calling the TopLevelComponent serialize, and leave a JSON wrapper that tells what type to use when deserializing on the client. Inside TopLevelComponent it would then detect the wrapper, and know how to deserialize. Done.

The question still remains how to specify the serialization/deseralization methods.

There are two proposals: Separate classes that use a naming convention, and providing an class level deserializer method, and instance level serialization method. I am fine with either one, but lean towards the latter approach (which also seems to be the way @wied03) is doing it.

@wied03
Copy link
Contributor

wied03 commented Mar 29, 2016

OK. I have no opinions about how some of this works since I won't use it. As long as a lot of this caching code, etc. (assuming caching was created for serialization as @ajjahn explained) is moved to TopLevelComponent and TopLevelComponent is relocated to one of the other GEMs, I'm happy.

@catmando
Copy link
Collaborator

@ajjahn - RE: your comments over on #134 - I want to be sure we are on the same page here just we don't get confused. This has nothing to do with reactive-record. I simply believe that there needs to standardized way to pass data from server to client, and have the data arrive in the opal component in the same form as it left the client. I.e. serialized then deserialized.

This is a function of the code being isomorphic, thus anybody getting into react.rb will likely have an expectation that if I do something like this:

render_component "Foo", some_param: MyBigClass.new(...)

that inside the component Foo, some_param should be an instance of MyBigClass.

@wied03 - I think the direction is to move TopLevelComponent in with some kind of isomorphic helpers gem, that can be used to be (for example) a rails interface.

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

No branches or pull requests

3 participants