-
Notifications
You must be signed in to change notification settings - Fork 1
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
Default to JSON instead of Jason in Elxir 1.18.x #73
Conversation
@@ -161,7 +161,7 @@ defmodule Charon.TokenFactory.Jwt do | |||
%{get_keyset: get_keyset, signing_key: kid} = get_mod_config(config) | |||
|
|||
with {:ok, key = {alg, _secret}} <- config |> get_keyset.() |> get_key(kid), | |||
{:ok, json_payload} <- jmod.encode(payload) do |
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.
JSON only has encode!/1
and there's really no reason to use encode/1
here, since it would be a major programming error to have non-serializable input here.
lib/charon/config.ex
Outdated
@@ -58,7 +58,12 @@ defmodule Charon.Config do | |||
# 15 minutes | |||
access_token_ttl: 15 * 60, | |||
enforce_browser_cookies: false, | |||
json_module: Jason, | |||
json_module: | |||
if :erlang.system_info(:otp_release) |> to_string() |> String.to_integer() >= 27 do |
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 think this fails when running Elixir 1.17 on Erlang/OTP 27, which is supported.
Erlang/OTP added :json
in version 27, but Elixir added the JSON
module in 1.18. I would rather check against Elixir's minor version:
System.version() |> String.split(".") |> Enum.at(1) |> String.to_integer() >= 18
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.
hm good point, I got confused because they added native json support to erlang originally, but of course the elixir version has to actually have the module too 🤦♂️
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.
Elixir's JSON
implementation doesn't actually call Erlang's! It is fully independent, albeit partially copied from the OTP implementation: https://github.com/elixir-lang/elixir/blob/main/lib/elixir/src/elixir_json.erl.
No description provided.