-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Bid caching flag #3402
Bid caching flag #3402
Conversation
@@ -18,6 +18,7 @@ const DEFAULT_BIDDER_TIMEOUT = 3000; | |||
const DEFAULT_PUBLISHER_DOMAIN = window.location.origin; | |||
const DEFAULT_ENABLE_SEND_ALL_BIDS = true; | |||
const DEFAULT_DISABLE_AJAX_TIMEOUT = false; | |||
const DEFAULT_BID_CACHE = true; |
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.
The Prebid Board would like the default to be "no cache", so that would change this default to false
, right?
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.
In your e-mail you said the board wanted the ability to "opt-out". If I changed the default to false
wouldn't that be "opt-in" in regards to bid caching?
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.
My recollection of the board discussion was publishers would have to opt-in and we'd disable by default
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.
believe we decided to add the flag as the 1st step, then as a 2nd step making it off by default (breaking change).
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.
@mkendall07 that's what I recall as well, which is how it is implemented now. Also, switching it to off will require some effort more than just changing that constant. Since many tests related to targeting assume bid caching, they will have to be updated to behave appropriately.
@bretg let me know if this is okay as is or if we want to further discuss switching the default.
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.
I updated the docs PR with a warning that the default will be changing in the coming releases.
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.
Then we ship it as a 2.0. We are not making breaking changes without major version changes period.
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.
So sounds like this is the plan, right?
1.x - leave bid-caching on by default, but add a config flag that will allow a publisher to turn it off
2.0 - turn bid-caching off by default and let pubs turn it on with a config flag.
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.
correct, thanks. @bretg if you have reason to push this out sooner than an expected release for 2.0, we should talk about how to handle. I.E if we make it the only change in 2.0 that's probably OK but it creates additional overhead to support legacy for 30 days per our policy (vs releasing a bigger 2.0 release)
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.
The board wants this change done 3 months ago.
So if we want to increment the version as a result of this change, so be it. Let's plan for a 2.0 by the end of the month then?
bf83756
to
90da819
Compare
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
* remove comment that isn't relevant anymore. tll in targeting code * bid caching option added (default on) * fix typo in config name
Type of change
Description of change
This adds a new flag,
useBidCache
, for disabling bid caching. The default foruseBidCache
is true.Other information
Documentation pull-request here: prebid/prebid.github.io#1069
fixes #2992