Use Jason instead of Poison for json encoding #2734

chrismccord · 2018-01-30T00:07:10Z

josevalim · 2018-01-30T00:25:14Z

lib/phoenix/controller.ex

  defp get_json_encoder do
    Application.get_env(:phoenix, :format_encoders)
-    |> Keyword.get(:json, Poison)
+    |> Keyword.get(:json, Jason)


This is backwards incompatible, I am afraid.

Yeah, I was going to ask how we should handle it. We can make it an explicit config, but then we'd still need to ship with the poison* dep, which is not ideal

We cannot simply change to Jason as that is backwards incompatible, since Jason is not fully compatible with Poison.

In my opinion, we should expose this function behind Phoenix.json_encoder (it can be @doc false) and use it throughout the codebase but we should still default to poison. In new apps, we generate:

config :phoenix, :format_encoders, json: Jason

Another reason to this clean up is that Ecto can only migrate to Jason by default in 3.0. So I am afraid that making this easily achievable is the best way to go for now.

josevalim · 2018-01-30T00:25:45Z

lib/phoenix/template.ex

  alias Phoenix.Template

-  @encoders [html: Phoenix.Template.HTML, json: Poison, js: Phoenix.Template.HTML]
+  @encoders [html: Phoenix.Template.HTML, json: Jason, js: Phoenix.Template.HTML]


Also backwards incompatible.

FWIW, this is still backwards incompatible. :) It should be Phoenix.json.

josevalim · 2018-01-30T00:26:18Z

lib/phoenix/transports/v2/longpoll_serializer.ex

      %Message{topic: msg.topic, event: msg.event, payload: msg.payload}
      |> to_list()
-      |> Poison.encode!()
+      |> Jason.encode!()


We should start using encode_to_iodata here.

josevalim · 2018-01-30T00:26:26Z

lib/phoenix/transports/v2/longpoll_serializer.ex

  """
  def encode!(msg) do
-    {:socket_push, :text, msg |> to_list() |> Poison.encode!()}
+    {:socket_push, :text, msg |> to_list() |> Jason.encode!()}


Encode to iodata.

michalmuskala · 2018-01-30T00:19:35Z

guides/docs/controllers.md

 Assuming we had a route for `get "/our_path/:id"` mapped to this `show` action, going to `/our_path/15` in your browser should display `Showing id 15` as plain text without any HTML.

-A step beyond this is rendering pure JSON with the `json/2` function. We need to pass it something that the [Poison library](https://github.com/devinus/poison) can parse into JSON, such as a map. (Poison is one of Phoenix's dependencies.)
+A step beyond this is rendering pure JSON with the `json/2` function. We need to pass it something that the [Jason library](https://github.com/michalmuskala/jason) can parse into JSON, such as a map. (Jason is one of Phoenix's dependencies.)


Should this be "encode" instead of "parse"?

michalmuskala · 2018-01-30T00:23:49Z

lib/phoenix/transports/v2/longpoll_serializer.ex

      %Message{topic: msg.topic, event: msg.event, payload: msg.payload}
      |> to_list()
-      |> Poison.encode!()
+      |> Jason.encode!()


We might use encode_to_iodata! here as well, like in the websocket serializer.

michalmuskala · 2018-01-30T00:25:22Z

test/phoenix/transports/websocket_serializer_test.exs

+  @v1_msg_json ["{\"", [[], "event"], "\":", [34, [], "e", 34], ",\"", [[], "payload"], "\":", [34, [], "m", 34], ",\"", [[], "ref"], "\":", "null", ",\"", [[], "topic"], "\":", [34, [], "t", 34], 125]
+  @v1_reply_json ["{\"", [[], "event"], "\":", [34, [], "phx_reply", 34], ",\"", [[], "payload"], "\":", ["{\"", [[], "response"], "\":", "null", ",\"", [[], "status"], "\":", "null", 125], ",\"", [[], "ref"], "\":", [34, [], "null", 34], ",\"", [[], "topic"], "\":", [34, [], "t", 34], 125]
+  @v1_fastlane_json ["{\"", [[], "event"], "\":", [34, [], "e", 34], ",\"", [[], "payload"], "\":", [34, [], "m", 34], ",\"", [[], "ref"], "\":", "null", ",\"", [[], "topic"], "\":", [34, [], "t", 34], 125]
+  @v2_msg_json [91, "null", 44, "null", 44, [34, [], "t", 34], 44, [34, [], "e", 34], 44, [34, [], "m", 34], 93]


Would it make sense to flatten the iodata before assertion in the test? It should be less brittle in case of changes in the library.

chrismccord · 2018-01-30T01:23:10Z

Recap:

Require explicit configuration of json format_encoder for Phoenix to boot. (may also default to Poison)
Use explicit config for new applications that configures Jason in user's mix config.
Add :jason to new application's deps.
Add :jason as optional dep in phoenix (tests)
Drop Poison from Phoenix deps

chrismccord · 2018-02-01T03:11:38Z

@josevalim updated as discussed. Can you take another look? Thanks!

chrismccord · 2018-02-01T03:13:21Z

lib/phoenix.ex


    children = [
+      # Ensure format encoders are loaded before starting
+      worker(Task, [fn -> ensure_format_encoders() end], restart: :transient),


I'm not thrilled about this approach but it's the only way to get reasonable error messages out of start. The downside is the default max_restarts shows the runtime error three times. Better ideas?

Would making it a temporary worker solve this?

Starting a worker sounds like too much.

@chrismccord if you are not happy with the error message when raising directly inside application, you have two options:

Log and return {:error, :missing_format_encoder}

Log and let the app boot anyway

josevalim · 2018-02-01T10:05:48Z

installer/templates/phx_single/config/config.exs

  metadata: [:request_id]

+# Use Jason for JSON parsing in Phoenix
+config :phoenix, :format_encoders, json: Jason


We also need to generate the configuration for Ecto:

config :ecto, :json_library, Jason

Is this deprecated? Should we generate config :#{db_adapter}, :json_library, Jason instead?

chrismccord · 2018-02-08T03:53:46Z

@josevalim can you take a final pass? I generated the "deprecated" for 3.0 Ecto config, but we can update to the ecto adapter config once 3.0 ships.

josevalim · 2018-02-08T08:48:19Z

installer/templates/phx_single/config/config.exs

+config :phoenix, :format_encoders, json: Jason
+<%= if ecto do %>
+config :ecto, :json_library, Jason
+<% end %>


OCD check: how is this going to be rendered in practice? Maybe it would be better to be defined as:

config :phoenix, :format_encoders, json: Jason<%= if ecto do %> config :ecto, :json_library, Jason <% end %>

?

josevalim · 2018-02-08T08:48:33Z

installer/templates/phx_umbrella/apps/app_name/config/config.exs

 use Mix.Config

 <%= if ecto do %>config :<%= app_name %>, ecto_repos: [<%= app_module %>.Repo]<% end %>
+<%= if ecto do %>config :ecto, :json_library, Jason<% end %>


We can probably merge the two conditionals into one.

josevalim · 2018-02-08T08:52:54Z

test/test_helper.exs

 Application.ensure_all_started(:cowboy)

+# Clear format encoder cache from startup and inject Jason
+Application.delete_env(:phoenix, :compiled_format_encoders)


Should we also cache Phoenix.json? Also, should we make it public?

Alternatively we can read Phoenix.json at compile time.

We can't read Phoenix.json at compile time because the application needs to be started to fetch application env, but isn't started yet ¯_(ツ)_/¯

No, we don't need to start the application to fetch the environment, only to fetch the defaults in mix.exs. But if the default is in the module, like in this case here, it should be dine.

Any configuration from the user application will also be available.

In any case, we can separate this discussion in two:

Shall we make this public?

Shall we cache it? At runtime or compile time?

Recap:

Introduce public Phoenix.json_library/0 with config :phoenix, :json_library configuration. No need to cache as runtime lookup is simple ets call.

josevalim · 2018-02-08T08:54:30Z

@chrismccord I have added some comments. Regarding Ecto 3.0, we have no estimate for its release, so I wouldn't worry about it.

Should we make Phoenix.json public? One advantage of doing so is that we don't need to hardcode Jason in the user application as well. For example, in Plug.Parsers we would pass Phoenix.json. One less place to change at the end of the day. Thoughts?

chrismccord mentioned this pull request Jan 30, 2018

Investigate using Jason instead of Poison #2693

Closed

josevalim reviewed Jan 30, 2018

View reviewed changes

michalmuskala reviewed Jan 30, 2018

View reviewed changes

chrismccord commented Feb 1, 2018

View reviewed changes

josevalim reviewed Feb 1, 2018

View reviewed changes

chrismccord force-pushed the cm-jason branch from 4d53a25 to 60af3d7 Compare February 8, 2018 03:51

josevalim reviewed Feb 8, 2018

View reviewed changes

chrismccord closed this Feb 9, 2018

chrismccord force-pushed the cm-jason branch from c818052 to 7be6a88 Compare February 9, 2018 18:54

chrismccord deleted the cm-jason branch February 9, 2018 18:59

Macavirus mentioned this pull request Jul 8, 2019

Replace Poison with Jason omgnetwork/ewallet#817

Open

elfenlaid mentioned this pull request Sep 17, 2025

Use the built-in JSON library #6481

Open

Uh oh!

Use Jason instead of Poison for json encoding #2734

Use Jason instead of Poison for json encoding #2734

Uh oh!

Conversation

chrismccord commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismccord Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismccord commented Jan 30, 2018

Uh oh!

chrismccord commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismccord commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrismccord Jan 30, 2018 •

edited

Loading

josevalim Feb 1, 2018 •

edited

Loading