-
Notifications
You must be signed in to change notification settings - Fork 750
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
Use jsonparser fork #637
Use jsonparser fork #637
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[[constraint]] | ||
branch = "master" | ||
name = "github.com/buger/jsonparser" | ||
source = "https://github.com/dbemiller/jsonparser.git" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this fork to live in the prebid github org? Probably not worth it if we're intending to switch back to upstream after a single PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... I considered that too. I chose not to put it in the prebid org just because I didn't want to pollute the namespace. Hopefully this is a short-lived fork.
We can always move it if it turns out to be a long-lived fork and we care that much. At this point, we don't even know for certain that this will get rid of the panics, or whether it will cause other bugs... so that seemed a little premature.
This switches our project to a
jsonparser
fork which includes buger/jsonparser#137, because that project's maintainer hasn't responded in a while.I also tried replacing
jsonparser
with gjson... but our benchmark showed an extra millisecond of overhead, so... shrugs.I believe this should fix some panics we've been seeing in the app logs. Once it merges we can switch back to the main project.
It upgrades lots of other packages too. Honestly this happened by accident when I deleted
Gopkg.lock
and randep ensure
. If you think that's too risky, I can go back and update just this package... but IMO we're due for some dependency upgrades anyway.