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

Allow customizing the JSON encoder for channels. #2631

Closed
michalmuskala opened this issue Nov 12, 2017 · 9 comments
Closed

Allow customizing the JSON encoder for channels. #2631

michalmuskala opened this issue Nov 12, 2017 · 9 comments

Comments

@michalmuskala
Copy link
Contributor

Today channels are hard-coded to use Poison. While this is convenient from the implementation's point of view, there might be different reasons to use other implementations (most frequently performance).

It would be great, if it was possible to somehow configure it, similar to how we can configure the JSON encoder for the HTTP part of Phoenix.

Additionally, the encoders should probably use the encode_to_iodata! function, similar to the HTTP layer to keep consistency.

@webdeb
Copy link
Contributor

webdeb commented Nov 12, 2017

@michalmuskala I like the idea, the encoder should be format agnostic in general, I've added a similar question about the Phoenix.Token encoder. #1996 (comment)

If we keep the function names consistent, it would allow us to use the same encoder everywhere.

@chrismccord
Copy link
Member

This is doable today by writing your own serializer. For example, the following serializer would be for jiffy and usable via : transport :websocket, Phoenix.Transports.WebSocket, serializer: JiffySerializer

defmodule MyApp.JiffySerializer do
  @behaviour Phoenix.Transports.Serializer

  alias Phoenix.Socket.{Reply, Message, Broadcast}

  @doc """
  Translates a `Phoenix.Socket.Broadcast` into a `Phoenix.Socket.Message`.
  """
  def fastlane!(%Broadcast{} = msg) do
    data = :jiffy.encode([nil, nil, msg.topic, msg.event, msg.payload])
    {:socket_push, :text, data}
  end

  @doc """
  Encodes a `Phoenix.Socket.Message` struct to JSON string.
  """
  def encode!(%Reply{} = reply) do
    data = [reply.join_ref, reply.ref, reply.topic, "phx_reply",
            %{status: reply.status, response: reply.payload}]
    {:socket_push, :text, :jiffy.encode(data)}
  end

  def encode!(%Message{} = msg) do
    data = [msg.join_ref, msg.ref, msg.topic, msg.event, msg.payload]
    {:socket_push, :text, :jiffy.encode(data)}
  end

  @doc """
  Decodes JSON String into `Phoenix.Socket.Message` struct.
  """
  def decode!(raw_msg, _opts) do
    [jref, ref, topic, event, pay | _] = :jiffy.decode(raw_msg, [:return_maps])

    %Phoenix.Socket.Message{
      topic: topic,
      event: event,
      payload: pay,
      ref: ref,
      join_ref: jref,
    }
  end
end

I know this is slightly more work than a couple encode functions, but given you need to wrap a module yourself for your desired JSON lib, does this suffice?

@michalmuskala
Copy link
Contributor Author

Awesome that's already possible!

I'm just not sure I see the reason why switching the JSON implementation for channels should be that differenr from switching the implementation for the HTTP part or ecto or postgrex. Everywhere it's usually a one-line configuration and the required interface is exactly the same - decode/1 and encode_to_iodata!/1. This means one needs to add yet another special module just for this (and figuring out how it should look like is not straightforward).

@webdeb
Copy link
Contributor

webdeb commented Nov 13, 2017

Let me mention (again), that it's not configurable for Phoenix.Token payload :p yet.

So, if we think about encoding any kind of payload, it make sense to have a common API for this. I am unsure about encode_to_iodata! & decode, instead I think that term_to_binary / binary_to_term functions, which are already built into erlang, are a better choice. ...the (hard)-naming problem is already solved here.

@ericmj
Copy link
Contributor

ericmj commented Nov 13, 2017

The encode_to_iodata! and decode names are already established convention, I don't see anything gained by changing them at this point.

term_to_binary cannot be used directly anyway because you would also need to base64 encode it since the websockets expect text here.

@webdeb
Copy link
Contributor

webdeb commented Nov 13, 2017

The encode_to_iodata! and decode names are already established convention, I don't see anything gained by changing them at this point.

Ok, I see. ...however, this convention could then also followed inside the Phoenix.Token encoder, no?

term_to_binary cannot be used directly anyway because you would also need to base64 encode it since the websockets expect text here.

this line, I don't get... I thought binary == string/text, or what do you mean here?

@ericmj
Copy link
Contributor

ericmj commented Nov 13, 2017

this line, I don't get... I thought binary == string/text, or what do you mean here?

Phoenix sends websocket text frames where only utf8 encoded strings are allowed. Sending arbitrary binary data in those frames is not allowed.

@michalmuskala
Copy link
Contributor Author

@chrismccord What do you think about allowing setting the serializer option to take something like {module, extra} and all calls be made to apply(module, callback, args ++ extra)?

Unfortunately this means the behaviour does not work that well 😒 .

An alternative could be to introduce changes in the behaviour to expect /2 functions instead of /1 with a warning and a function_exported check to call the /1 version to remain backwards compatible.

@chrismccord
Copy link
Member

@michalmuskala For now I think the best option is to keep the existing serializer configuration and folks that want to use a different JSON lib can either add a 20 line module, or better yet include a jsonthing_phoenix that ships the module. Then it's only a 1 line change in their socket

I'm just not sure I see the reason why switching the JSON implementation for channels should be that differenr from switching the implementation for the HTTP part or ecto or postgrex. Everywhere it's usually a one-line configuration and the required interface is exactly the same

I agree with this point and I would be open to thinking about it with 2.0, but for now I would prefer to wait so we don't have to do any dancing around extra options or similar. Thanks!

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

No branches or pull requests

4 participants