-
Notifications
You must be signed in to change notification settings - Fork 2
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
Swap JSX for Euneus #11
Conversation
It looks like dialyzer does not see your json parser. Is it in the app.src? In other instances I found the same sort of issue. |
Thanks for the tip @mmzeeman, but there was an issue in Euneus spec. I will wait for feedback until I clean up the PR removing the duplicated files and the benchmark project. |
This looks like an impressive improvement! Great work. |
The PR is ready to be reviewed. |
@@ -2,7 +2,7 @@ | |||
{description, "JSX wrapper to handle records and 'undefined'"}, | |||
{vsn, "git"}, | |||
{registered, []}, | |||
{applications, [ kernel, stdlib, syntax_tools, compiler, jsx ]}, | |||
{applications, [ kernel, stdlib, syntax_tools, compiler ]}, |
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.
Maybe add euneus here?
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.
Euneus does not need to be started as an application. I think it's not necessary to add it there.
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.
A couple of months ago I had to add dependencies there because dialyzer couldn't find types if I didn't. But reading from erlang documentation, the applications entry is indeed intended for applications which must be started before this application.
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.
@mmzeeman I think I had the same issue with Dialyzer by using cowboy
, but adding the plt_extra_apps
option as below solved it:
% rebar.config
{dialyzer, [
{plt_extra_apps, [cowboy]}
]}.
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.
Ah... but cowboy
is an application which must be started. It's a bit strange that as a user of an application you have to know how it works internally.
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 don't remember correctly, maybe I have made a mistake, but I was using ErlangLS extension for VSCode and cowboy
behaviors were not being recognized.
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.
We had a similar problem. The CI scripts suddenly all failed. I didn't know about the dialyzer config at the time.
Question @mworrell. What about other deps we use which uses jsx
. For core zotonic that is probably mqtt_sessions
. Should we change the default json encoder there too? There are also a couple of other places where it is used directly.
{euneus, "0.6.0"} | ||
]}. | ||
|
||
{dialyzer, [ |
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.
Ah.. @mworrell here you add the dependency for dialyzer. If it is listed in applications
in the app.src
file it is automatically picked up.
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.
Excellent... 🎉
This almost means a rename of this project. Maybe it should become json_record
at some point.
This PR proposes swapping JSX for Euneus for performance and memory reasons.
Motivation
Zotonic largely does JSON parsing and generation, so it's extremely important to be performant.
What is Euneus?
Euneus it's a JSON parser and generator. It is a rewrite of Thoas, but it's more flexible and performant.
Benchmarks
Setup
Results
Encode
Decode