Remove Poison as dependancy #28
Conversation
lib/chartkick.ex
Outdated
| end | ||
|
|
||
| defp options_json(opts) when is_bitstring(opts) do | ||
| opts | ||
| end | ||
|
|
||
| defp json_serializer do | ||
| Application.get_env(:chartkick, :json_serializer) || Poison |
There was a problem hiding this comment.
Wouldn't Poison cause problems if the user doesn't include it as a dependency (since its been removed as a dependency from here)?
There was a problem hiding this comment.
Great catch, that means that we have a couple of options:
Option 1:
We might need to throw back an error to the user if the json_serializer they have defined is not available?
Option 2:
We can instruct the user to install the serializer together with the chartkick dependancy.
There was a problem hiding this comment.
Hm.. If we go with option 2 and the user forgets to install the serializer the error would be weird/confusing.
I'd option 1 since that would enable us to give the user a reasonable error message were we could ask them to install and configure a serializer properly.
Thoughts?
There was a problem hiding this comment.
Yeah, sure that is better user experience too. Let me try something out and push
|
LGTM 👍 Merging this. Thanks a lot @sigu 🙌 Unrelated question |
|
One thing I just realized is that this change isn't backwards compatible 🤔 In previous versions this would add I will update the |
Since we are allowing the user to choose either
PoisonorJasonas the decoding library, we can removePoisonas a dependency and allow the user to install an encoder of their preferenceWe also add
eexinto extra applications to remove elixir1.11warnings