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

Load JSON at runtime #2269

Merged
merged 2 commits into from Aug 31, 2020
Merged

Load JSON at runtime #2269

merged 2 commits into from Aug 31, 2020

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented May 17, 2020

Description

Fixes loading JSON when Gemfile contains JSON versions that differ from the default gem or previously loaded versions. Various versions of Bundler have had issues with default gems, as it also loads some of them.

This PR changes JSON loading to runtime.

Closes #2206

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@ylecuyer
Copy link
Contributor

It is working with my reproduction example ;)

I have mixed feelings about this PR as it fixes the issue but isn't really future proof

@MSP-Greg
Copy link
Member Author

but isn't really future proof

In what sense? Loading/requiring at runtime is essentially the same as autoload.

@ylecuyer
Copy link
Contributor

What I meant was if someone in a future commit adds a require 'json' on top of a new file we won't notice it until it breaks again

@MSP-Greg
Copy link
Member Author

Given the possible issues with default gems, I think it would be worthwhile to autoload json, psych/yaml, and openssl. Using autoload removes the need for adding multiple requires when loading at runtime.

I'll have a look at all three.

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor bug labels May 21, 2020
@nateberkopec
Copy link
Member

I'm gonna roll with this for now, we can do a longterm fix later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug important! waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in 4.3.3 with require json
3 participants