Skip to content

Conversation

@altjohndev
Copy link

Sadly, the JSON kernel module does not provide the encode/1 function, which impacts the usage of this library.

In order to match the kernel implementation, this PR changes the interface from decode/1 and encode/1 to decode!/1 and encode!/1.

This PR does not change the default library (keeping it as Jason).

It will use JSON for tests if the Elixir version is 1.18+. The CI tests both libraries.

Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

I’m thinking maybe a better approach for this would be:

  1. Introduce a private module, Sentry.JSON, which would act as a temporary middleman (until we will depend on Elixir 1.18+).
  2. Check the Elixir version when compiling Sentry and
    1. Use JSON if on Elixir 1.18+, no configuration (after all, we'll eventually drop :json_library now that JSON is in stdlib) + warn if :json_library is configured. This also means we don't need to switch to encode! and decode!, we know which lib we're using.
    2. Use the configured :json_library if on earlier versions.

Thoughts?

@altjohndev
Copy link
Author

altjohndev commented Jan 2, 2025

Hi @whatyouhide , applied the suggested changes.

Use JSON if on Elixir 1.18+, no configuration (after all, we'll eventually drop :json_library now that JSON is in stdlib) + warn if :json_library is configured. This also means we don't need to switch to encode! and decode!, we know which lib we're using.

It would be wise to still let developers define custom libraries during this transitioning period.

@whatyouhide
Copy link
Collaborator

It would be wise to still let developers define custom libraries during this transitioning period.

Why? I can only think of custom protocols and stuff like that, but maybe that's already enough.

@altjohndev
Copy link
Author

Why? I can only think of custom protocols and stuff like that, but maybe that's already enough.

Just like this library was not ready to use the JSON module, many other libraries and services will require adjustments.

For example, Some services/libraries require a transition for structs that derive from Jason (or another encoder) to derive to JSON instead.

@whatyouhide
Copy link
Collaborator

many other libraries and services will require adjustments

True, but using JSON alongside other JSON library would totally be fine. Either way, I get the point: let's keep the configuration option then, but change the default to JSON. This is a bit of a breaking change but one we can deal with IMO, we just advise people to explicitly set the :json_library option if they're on 1.18 but don't want to use JSON.

@altjohndev
Copy link
Author

Thank you for the advice. 🙇

Let me know if I need to change or improve something.

@whatyouhide
Copy link
Collaborator

Closing in favor of #845.

@whatyouhide whatyouhide closed this Jan 3, 2025
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.

2 participants