Repository navigation
Remove jsx use otp27 json - #261
Conversation
|
Code change looks good to me, but CI didn't run. |
|
@kivra-kenand thanks! Can you please update |
|
Yes sorry, been on vacation (silly Sweden). Looking into it now! |
I see that CHANGELOG does not have 2.5.0 nor 2.6.0 in it, and it is to be generated after a merge+tag. Do you want me to add a manual ## BREAKING CHANGE at the top without a tag? I am unsure how that would play along with auto-generated docs via github_changelog_generator tho. |
I believe that we stopped using CHANGELOG.md a long time ago… when github releases started to generate release notes on their own. I think those are more useful than CHANGELOG.md, anyway. |
Why
jsxencodes one notification map and two small JWT objects — and it is the worst tool available for it on two counts.It is unmaintained.
Last Hex release is 3.1.0 from April 2021, over five years ago. The repo is not formally archived and saw a commit in mid-2024, but 25 issues are open and nothing has shipped. It is a dependency that ~73M downloads inherit and nobody is steering.
It is slow.
jsx is pure Erlang with no NIF and is widely reported as the slowest of the mainstream Erlang JSON libraries. Measured on this library's own payloads (OTP 28.4.2, 200k iterations,
iolist_to_binaryincluded so it is like-for-like):jsonFor a push provider on a hot path, paying 4-7x on every notification for a stalled dependency makes no sense once stdlib does the same job. This drops the runtime dependency list from three to two (
gun,base64url).The cost is the OTP 27 floor: this drops OTP 24-26, which the old
minimum_otp_vsnallowed.That is breaking and wants a version bump — the number is yours to pick.
What changed
jsx -> json.
Both push paths use
iolist_to_binary(json:encode(_)); the wrap is required becausejson:encode/1returnsiodata().generate_token/3builds its JWT header and payload as maps — unlike jsx,json:encode/1rejects proplists with{unsupported_type, _}. jsx dropped fromrebar.config,rebar.lockandapplications;minimum_otp_vsn24 -> 27; CI matrix 27/28/29, which also neededrebar3(3.22.1 cannot build on 28+),checkoutandsetup-beambumped.Tests for the encoding.
push_notification_json_bodyasserts the body handed togun:post/4round-trips viajson:decode/1across binary keys, atom keys, nesting and integers, and asserts itis_binary/1— the guard for theiodata()pitfall.generate_token_jwt_claimsdecodes the JWT and checks its claims, guarding the proplist-to-map change.ctre-enabled.It was disabled in 3adaba0 because the suite still mocked
gun:await/3andgun:await_body/3, APIsapns_connectionstopped calling when responses moved to{gun_response, _}/{gun_data, _}messages — those tests were waiting for a reply that could never arrive. The mocks now drive the async API. Also:push_notification_timeoutexpected a baretimeoutwhere the code returns{error, timeout}, andapns_meta_SUITEis deleted — it wrapped xref/dialyzer/elvis, all already separate workflow steps, and was failing on a katana_testelvis_coremismatch (undef export_used_types) plus a missing PLT.That drops
katana_testandmixeras test deps.Two calls worth extra attention
{error, not_connection_owner}is gone.The README said pushes must come from the creating process, but nothing has enforced it since requests became per-stream with their own
From, which made multi-process pushing safe. The stale test was the last thing asserting it.It is replaced by
push_notification_from_any_process, which proves the current behaviour, and the README now documents it. Say the word and I will restore the guard instead.response()andjson()corrected.The new tests made dialyzer fail, all of it tracing to specs that did not match the code: the body is
binary()not[term()]; timeouts return{error, term()}, not the baretimeoutthat was never returned; andjson()excluded the atom keys the README's own examples use. Fixing the types beat loosening the tests, but this changes exported types, so it is the easiest thing here to split out.Verification
xref,dialyzer,hank,lint,ctall clean; 19 tests pass. Local runs on OTP 28.4.2 / rebar3 3.26.0; the matrix covers 27-29. The encoding test is mutation-checked — removingiolist_to_binarymakes it fail.Deliberately not included
gundeliberately left at 2.1.0: worth bumping, but gun is fully mocked here so a bump would stay green even if the transport regressed.