Skip to content

Conversation

@LasseBlochWH
Copy link
Contributor

@LasseBlochWH LasseBlochWH commented Aug 18, 2025

This allows ja_serializer to run on OTP-28, pulls inflex-lib directly into library, and updated dependencies, adds CI.

OTP 28 support

Erlang/OTP 28 no longer allows regexes to be defined in the module body and interpolated into an attribute, this meant that the depency inflex cannot be compiled under OTP-28. there is an open pr which fixes the issue.

inflex now part of our ja_serializer fork

I've opted to remove inflex as a dependency (Clone of the branch which fixes OTP 28 compatibility) and added it directly to ja_serializer see a41c595. 'inflex' have not been updated in 5 years see hex.pm.

CI

workflow have been added, so we now executed the units test and lint on each push.
This required some code changes (credo and formatting)
Note: otp-28 elixir 1.17.x is excluded in matix (otp-28 is first supported from elixir 1.18.4

Deps

The deps have been updated as well

@LasseBlochWH LasseBlochWH self-assigned this Aug 18, 2025
@LasseBlochWH LasseBlochWH added the area/backend Backend related tasks label Aug 18, 2025
Copy link
Contributor

@sebWH sebWH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skipped reviewing the Inflex code - I assume it was just copied from the fixed branch.

Some comments/questions:

  1. I see that we don't use Inflex.* anywhere in the Legolas code - this means it is used only by ja_serializer under the hood?
  2. I see some risks when it comes to usage of our ja_serializer fork. We can miss some important updates or improvements... But it looks like both ja_serializer and inflex are abandoned 🤔
  3. I saw this comment saying that with 28.1 the issue should be solved. I assume we don't want to wait for it and continue with this PR?
  4. Maybe this issue would be worth updating with a comment saying something about also including inflex in the fork alongside signifying that we will stick to the fork for a longer time 😏 Maybe the issue could be even closed?

@LasseBlochWH
Copy link
Contributor Author

I skipped reviewing the Inflex code - I assume it was just copied from the fixed branch.

Some comments/questions:

  1. I see that we don't use Inflex.* anywhere in the Legolas code - this means it is used only by ja_serializer under the hood?

Yes, its they only use we have of it now.

  1. I see some risks when it comes to usage of our ja_serializer fork. We can miss some important updates or improvements... But it looks like both ja_serializer and inflex are abandoned 🤔

I agree, it's not optimal that we have a fork, but if ja_serializer continues to be abandoned, we will see if we can move to another lib at some point, but who knows when we will get the chance for that. At least this way we can hopefully get something stable.

  1. I saw this comment saying that with 28.1 the issue should be solved. I assume we don't want to wait for it and continue with this PR?

Hmm 🤔 that's a great point. We are for once very bleeding edge with our packages. I personally would like to get it before 28.1 - there is as far as I can see no ETA on it: https://github.com/erlang/otp/milestone/218.
We have used this period after the holidays to take some of the large chore-tasks like updating OTP. I would like to be done with that soonish, so I can return to road-mapped stuff.

  1. Maybe this issue would be worth updating with a comment saying something about also including inflex in the fork alongside signifying that we will stick to the fork for a longer time 😏 Maybe the issue could be even closed?

I've updated the issue with a comment. In regards to whether the issue could be closed - under all circumstances we will need to continue on our own forks as long as nobody is actively maintaining ja_serializer. But if somebody starts doing that, the issue makes sense.

Copy link
Member

@aamikkelsenWH aamikkelsenWH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉👍

@LasseBlochWH LasseBlochWH merged commit 807e641 into master Aug 20, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/backend Backend related tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants