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

only use JSON::XS if $ENV{PERL_JSON_XS_USE} #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mohawk2
Copy link

@mohawk2 mohawk2 commented Oct 5, 2017

No description provided.

@Grinnz
Copy link
Contributor

Grinnz commented Oct 5, 2017

For my use case, it would be ok if JSON::XS was still used when Cpanel::JSON::XS is not available, but Cpanel::JSON::XS was always preferred (and always installed if XS is possible). Requiring an env variable to activate JSON::XS seems like extra complication unless there is another reason to do that.

If an env variable is needed, it might be nicer to have a more generic one that specifies which backend(s) to prefer.

@plicease
Copy link

plicease commented Oct 18, 2018

I think it would be better to have an environmental override for the install and runtime detection, something like JSON_MAYBEXS_IMPLEMENTATION be one of JSON::XS, JSON::PP or Cpanel::JSON::XS. MaybeXS being clever has caused me a fair amount of grief in getting our development, CI and production environments all in sync because of lingering JSON::XS installs or dist install order issues.

@plicease
Copy link

To be clear, the cleverness is fine in the absence of an expressed preference, but there should be a way to express a preference.

@karenetheridge
Copy link
Member

Cleverness is generally not great. JSON::XS and Cpanel::JSON::XS have drifted too much.. e.g. karenetheridge/JSON-Schema-Modern#79

It's time to kill off the use of JSON::XS for good. Having an explicit environment variable akin to NO_REALLY_HIT_ME_HARDER would still allow for people to fall back to the old behaviour when absolutely needed.

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.

4 participants