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

fix: use common encode function across libraries #99

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

jhosteny
Copy link
Contributor

Ensure that we call a version of encode that exists for both the Jason library, and new builtin JSON module.

Fixes #98

Ensure that we call a version of encode that exists for both the Jason
library, and new builtin JSON module.

Fixes maxmarcon#98
@jhosteny
Copy link
Contributor Author

jhosteny commented Jan 28, 2025

Hi @maxmarcon. I am able to reproduce the bug in the library tests by adding the following to config/test.exs (with an updated version of elixir / erlang, of course):

config :phoenix, :json_library, JSON

I didn't put any test in, as I wasn't sure of how to accommodate both libraries easily, and the function is just missing.

@maxmarcon
Copy link
Owner

Hi @jhosteny thanks for the PR!

The built-in JSON module was introduced in Elixir 1.18 right? Then I think one way to test this is to:

  1. add Elixir 1.18 to the Elixir versions we're testing in the CI
  2. only configure the Jason as json_library if the Elixir version is < 1.18

1 is easy of course. Can 2 be done?

Dynamically select the Phoenix json library based on the current
version. For elixir 1.18 or greater, we choose the JSON module. For
prior versions, we choose Jason.
@jhosteny
Copy link
Contributor Author

jhosteny commented Jan 29, 2025

Hi @maxmarcon, thanks for taking a look! I added another commit which adds 1.18 to the CI matrix, and chooses JSON if it is version 1.18 or higher, and Jason otherwise.

If this looks okay, let me know if you'd like me to squash the commits.

@maxmarcon
Copy link
Owner

Thanks @jhosteny, this looks great! I'll merge and release a new version as soon as I get the chance

@maxmarcon maxmarcon merged commit 4c415be into maxmarcon:main Jan 29, 2025
5 checks passed
@maxmarcon
Copy link
Owner

🥳 🚀

@jhosteny
Copy link
Contributor Author

Thank you so much @maxmarcon!

@maxmarcon
Copy link
Owner

Thanks to you! You created the MR 😄

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.

Version 1.5.3 call non-existing function when configured with JSON
2 participants