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

Bid caching flag #3402

Merged
merged 3 commits into from
Jan 18, 2019
Merged

Bid caching flag #3402

merged 3 commits into from
Jan 18, 2019

Conversation

snapwich
Copy link
Collaborator

@snapwich snapwich commented Dec 20, 2018

Type of change

  • Feature

Description of change

This adds a new flag, useBidCache, for disabling bid caching. The default for useBidCache is true.

// disable bid caching
pbjs.setConfig({
  useBidCache: false
});

Other information

Documentation pull-request here: prebid/prebid.github.io#1069

fixes #2992

@@ -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;
Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

Copy link
Contributor

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

Copy link
Member

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).

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Member

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.

Copy link
Collaborator

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.

Copy link
Member

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)

Copy link
Collaborator

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?

@harpere harpere added needs review needs update needs 2nd review Core module updates require two approvals from the core team question and removed needs update labels Jan 3, 2019
@bretg bretg removed the question label Jan 4, 2019
@harpere harpere added needs update 2.0 API Change and removed needs 2nd review Core module updates require two approvals from the core team needs review labels Jan 4, 2019
@snapwich snapwich self-assigned this Jan 7, 2019
@snapwich snapwich added needs 2nd review Core module updates require two approvals from the core team and removed needs update labels Jan 18, 2019
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM

@mkendall07 mkendall07 merged commit 53517d3 into master Jan 18, 2019
amuraco pushed a commit to amuraco/Prebid.js that referenced this pull request Jan 28, 2019
* remove comment that isn't relevant anymore.  tll in targeting code

* bid caching option added (default on)

* fix typo in config name
pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019
* remove comment that isn't relevant anymore.  tll in targeting code

* bid caching option added (default on)

* fix typo in config name
jacekburys-quantcast pushed a commit to jacekburys-quantcast/Prebid.js that referenced this pull request May 15, 2019
* remove comment that isn't relevant anymore.  tll in targeting code

* bid caching option added (default on)

* fix typo in config name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide control for the publisher to turn off bid caching
5 participants