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

[dependency upgrades] Poison replaced with jason and httpoison upgrade #50

Merged
merged 1 commit into from
Nov 4, 2018

Conversation

izelnakri
Copy link
Collaborator

No description provided.

@ayrat555
Copy link
Member

ayrat555 commented Nov 3, 2018

Is there any benefit of using Jason over Poison?

@izelnakri
Copy link
Collaborator Author

Yes allegedly its faster and used instead of poison on ecto v3

@InoMurko
Copy link
Member

InoMurko commented Nov 3, 2018

Is Poison used more then Jason? If yes, that would mean most projects that include exthereum as a dependency would now have two json encoders/decoders. The PR should rather chose an approach that would make the project independent of a particular choice.

@hayesgm
Copy link
Collaborator

hayesgm commented Nov 3, 2018

It would be great for us to benchmark. The only concern is that projects that use this are now likely going to pull in a second JSON parsing library (or, on the other hand, go from two to one...)

@InoMurko
Copy link
Member

InoMurko commented Nov 4, 2018

What if we do something like Phoenix does with pluggable decoders?
https://github.com/elixir-plug/plug/blob/master/lib/plug/parsers.ex#L271

@izelnakri
Copy link
Collaborator Author

Yes, I thought about this as well, ecto does something similar: https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/json.ex#L9.

We can do the encoder thing as a seperate potential feature, however since ethereumex is not as widely used as ecto and there are several json libraries out there, do we really want to support all of them? They might have different APIs, options etc.

I dont mind if people have two json dependencies in their projects through subdependency resolution since this library will be mostly used in the application layer of a server. We should just use the fastest if they both have the good error messages,jason will eventually take over, as it seems for now.

@InoMurko
Copy link
Member

InoMurko commented Nov 4, 2018

Did a quick research and found out that Credo has jason as a dependency. Which means most projects that use Credo (like OmiseGO ewallet and OMG Network), will have Jason already there.

@izelnakri izelnakri merged commit 3981a52 into mana-ethereum:master Nov 4, 2018
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

Successfully merging this pull request may close these issues.

4 participants