Add setConfig method #1325

matthewlane · 2017-06-26T21:51:51Z

Type of change

Feature
Refactoring

Description of change

Adds pbjs.setConfig and pbjs.getConfig api methods for working with Prebid configuration.

Usage:

pbjs.setConfig({ debug: <true|false> }); to enable/disable logging (this will replace pbjs.logging = <true|false>, which still works for now but will be depreciated).
pbjs.setConfig({ <key>: <value> }); for setting arbitrary configuration

The getConfig function is for retrieving the current configuration object or subscribing to configuration updates. When called with no parameters, the entire config object is returned. When called with a string parameter, a single configuration property matching that parameter is returned.

config.getConfig() => get config object
config.getConfig('debug') => get debug config

The getConfig function also contains a 'subscribe' ability that adds a callback function to a set of listeners that are invoked whenever setConfig is called. The subscribed function will be passed the options object that was used in the setConfig call. Individual topics can be subscribed to by passing a string as the first parameter and a callback function as the second

// subscribe to all configuration changes
getConfig((config) => console.log('config set:', config));

// subscribe to only 'logging' changes
getConfig('logging', (config) => console.log('logging set:', config));

// unsubscribe
const unsubscribe = getConfig(...);
unsubscribe(); // no longer listening

Other information

Implements #1247

dbemiller · 2017-06-27T14:38:56Z

Just to a quick sanity check...

As written, no prebid files will be able to read from the config "on load." Any getConfig calls will have to be nested inside functions which are only called through publisher-called functions... because otherwise we'll end up reading the config values before the page gets a chance to set them.

Did you guys already consider & accept that limitation?

mkendall07 · 2017-06-27T14:52:58Z

@dbemiller
yeah good call out. We have discussed it a bit and have talked about alternative approaches to have "on load" configs (data-* attributes for example) - but for the most part this is backwards compatitible since most of these values are set after the pbjs queue executes - we just expect the setConfig call to be made before the auction starts.

dbemiller · 2017-06-27T15:34:28Z

in that case... I need to challenge this.

pbjs.setConfig(cfg);
pbjs.thingWhichNeedsConfig();

can always be replaced by:

pbjs.thingWhichNeedsConfig(cfg);

The latter is less error-prone:

The caller can't screw up the ordering
The implementer can't use a variable they don't have yet

Easier to test:

No (explicit) global state which tests have to set/reset after each use
Each function's JSDoc can define only the cfg options it cares about. Publishers can define a single object, if we choose the expected property names well so that they don't conflict.

...and works more reliably if publishers want to run multiple auctions with different configs.

It's also possible to maintain backwards compatibility (in the short term). For example, we could start by implementing:

function thingWhichNeedsConfig(cfg) {
  $$PREBID_GLOBAL$$.debug = oldDebug;
  ... do stuff ...
}

Then we can rewrite the thingWhichNeedsConfig implementation to stop using the global state at our leisure, and then delete that first line in 1.0.

Are there any real advantages to setting the state beforehand?

dbemiller · 2017-06-28T18:29:45Z

Noting the discussion in standup in writing. There is one benefit of setConfig(). If the user makes many API calls without changing the config, then it's more convenient to call once:

pbjs.setConfig(cfg);
pbjs.doOneThing();
pbjs.doAnotherThing();
pbjs.doAThirdThing();

is a better user experience than:

pbjs.doOneThing(cfg);
pbjs.doAnotherThing(cfg);
pbjs.doAThirdThing(cfg);

I talked a bit more with @mkendall07 afterwards, and would like to modify the suggestion a bit. Suppose we do:

var configured = pbjs.withConfig(cfg);
configured.doOneThing();
configured.doAnotherThing();
configured.doAThirdThing();

Then the state has only been passed once, and we still maintain all the benefits I mentioned above.

snapwich · 2017-06-28T21:43:37Z

in the

var configured = pbjs.withConfig(cfg);
configured.doOneThing();
configured.doAnotherThing();
configured.doAThirdThing();

pattern, what is withConfig returning? Is it a separate instance than the pbjs global? If it is separate, how does that affect adapters and such that use the pbjs global, would they continue using a separate instance, or would they need to somehow get access to the configured instance?

dbemiller · 2017-06-28T23:36:32Z

Good question... a separate instance. In my view, withConfg may be the only function on the global. Nothing internal to prebid should ever use it.

The only two reasons I know of why adapters use the global today are:

To support JSONP
To check config values

(1) isn't supported in Prebid 1.0 anyway.
(2) could be handled by that registerAdapterLoader((cfg) => Adapter) pattern we talked about before.

If there are other reasons, though, please do let me know.

A more concrete example of core would look something like this:

// loader is a (cfg) => Adapter, and called in each adapter file.
export function registerAdapterLoader(loader) { }

$$PREBID_GLOBAL$$.withConfig = function(cfg) {
  var adapters = adapterLoaders.map(loader => loader(cfg));

  // This holds what we think of as the public prebid API today
  return {
    doAuction: function(auctionParams) {
      adapters.forEach( /* call bids & do other stuff here */ );
    }
  }
}

mkendall07 · 2017-07-17T16:17:31Z

We paused dev on this but will pick it up soon.

I'd like to propose we continue with the existing API for the following reasons:

Saves us time to completion - the alternative API with a pbjs instance would be a larger scope refactor
Maintaining backward compatibility before 1.0 is more difficult (I think).
Ease of publisher integration will be more difficult (more change points) with alternative API.

snapwich · 2017-07-24T16:59:59Z

src/config.js

+    utils.logError('setConfig options must be an object');
+  }
+
+  if (options.bidderTimeout) {


Can we make a convention that setConfig(obj) just extends extends the config object? like Object.assign(config, options) rather than individual assignment? Seems like the individual assignment could get verbose with a lot of options.

Right, was meaning to squash these, will use Object.assign, thanks

snapwich · 2017-07-24T17:01:50Z

src/config.js

+const DEFAULT_DEBUG = false;
+const DEFAULT_BIDDER_TIMEOUT = 3000;
+
+export let config = {


I think we need some way to subscribe to changes to this object. For instance, the currency module (in review still) will need to kick off a request to go and download the currency file as soon as its configuration is set. But without some sort of subscription model, it won't know when that is.

Added subscribe function, please review if this works for your use case

snapwich · 2017-07-26T18:56:31Z

src/config.js

+ * subscribe((config) => console.log('config set:', config));
+ * subscribe(({ debug }) => debug ? console.log('debug set:', debug) : '');
+ */
+export function subscribe(listener) {


I think listing to a label would be a slight improvement. Say I'm the currency module then I subscribe with

subscribe("currency", function() { ... });

and then I'm only alerted when the currency configuration is updated, and I'm only passed the relevant currency configuration.

// "currency" subscribe handler called pbjs.setConfig({ currency: { ... } }); // "currency" subscribe handler not called pbjs.setConfig({ bidderTimeout: 3000 })

That way if pbjs.setConfig is used multiple times to update different configs we clean up a lot of the noise of alerting every listener.

However, if the first argument to subscribe is the callback function (i.e. no label is specified) we could still have it behave as it does now where it gets called for any configuration change and sends the whole config.

dbemiller · 2017-07-27T15:09:58Z

src/prebid.js

@@ -745,6 +748,14 @@ $$PREBID_GLOBAL$$.setS2SConfig = function(options) {
  adaptermanager.setS2SConfig(config);
 };

+/**
+ * Set Prebid config options
+ * @param {object} options


This is gonna need a more specific JSDoc to be of much use to anyone. Remember we're trying to work towards #1408.

Check out the docs for an example of typedef and optional properties.

Couldn't quite parse how best to doc this, will try in future

dbemiller · 2017-07-27T15:11:15Z

src/prebid.js

+ * Set Prebid config options
+ * @param {object} options
+ */
+$$PREBID_GLOBAL$$.setConfig = function(options) {


Looks like this can just be $$PREBID_GLOBAL$$.setConfig = setConfig?

dbemiller · 2017-07-27T15:12:06Z

src/adaptermanager.js

@@ -246,7 +246,9 @@ exports.enableAnalytics = function (config) {
 };

 exports.setBidderSequence = function (order) {
-  _bidderSequence = order;
+  if (order === CONSTANTS.ORDER.RANDOM) {


Did you mean to change this behavior? It looks a little suspiciously out of place.

looks like it was moved from https://github.com/prebid/Prebid.js/pull/1325/files#diff-f8e049a1b6d4f9aa96fca41d2f7aa11dL697

I don't know why it did that check, but it doesn't look like this pull-request changes any functionality.

Right, intention was to consolidate the setBidderSequence behavior into adaptermanager rather than having it spread across two functions. Noticed this while looking at this function when adding the bidderSequence config. Open to dropping this change if it's too out of scope

dbemiller · 2017-07-27T15:15:50Z

src/prebid.js

@@ -645,8 +647,9 @@ $$PREBID_GLOBAL$$.setPriceGranularity = function (granularity) {
  }
 };

+/** @deprecated - use pbjs.setConfig({ enableSendAllBids: <true|false> }) */


Would recommend calling this sendAllBids instead, for better parallelism with other config options.

This name reads like "enable the send all bids feature"... which is less straightforward than "send all the bids"

Current name comes from https://gist.github.com/mkendall07/51ee5f6b9f2df01a89162cf6de7fe5b6#setconfig---new-api, ccing @mkendall07 for feedback

dbemiller · 2017-07-27T16:39:19Z

src/config.js

+
+const ALL_TOPICS = '*';
+
+let listeners = [];


Chatted about this a little bit offline... For the sake of testability, concurrent auctions, and infinite scroll, I think we should strongly avoid adding global, mutable state to the project. Especially in new modules.

I suggested an alternate pattern, which @matthewlane sounded to like too. I'm pretty sure it'll require a getConfig on $$PREBID_GLOBAL$$, which kinda sucks... but I think it's a worthwhile tradeoff.

I can describe it here if anyone else cares or wants to hear it.

Can you describe what you're thinking? I think what we have here is pretty good.

I don't really consider this global state as its encapsulated to the config module; however, it is "side-effecty", but I'm not sure that's a terrible thing. However, it does need a way to clean up its side-effects for testing purposes, i.e. we need some way to remove listeners.

In this file:

export function newConfig() { let listeners = []; return { getConfig() { ... } setConfig(options) { ... } subscribe(topic, listener) { ... } }; }

In prebid.js:

import { newConfig } from './config'; const config = newConfig(); $$PREBID_GLOBAL$$.getConfig = config.getConfig $$PREBID_GLOBAL$$.setConfig = config.setConfig

In modules:

const getConfig = $$PREBID_GLOBAL$$.getConfig

Unit tests don't need to worry about side-effects because they can call newConfig() each time.

I agree that what we have here is workable. Honestly, the question which is really in the back of my mind is "how do we get to an API like @protonate suggested, without it being such a massive project?" Something like:

prebid.push(function() { prebid.doAuction().then(renderAd); }

As written, this config module can only be used in a project which is driven by global state. That's fine... because the current project is driven by global state.

Using my suggestion, this config module could be used in both systems.

function doAuction() { const config = newConfig(); ...auction impl here... }

If we keep adding new modules which rely on global state, then certain features (like infinite scroll) become more and more difficult to support with time.

If we add modules which don't rely on global state, then we can reuse them out of the box if(/when) we migrate in that direction.

I don't think this would really gain us anything. You would have to move the let config into the newConfig closure as well, right? So then we still have a "global" state for Prebid.js, it's just inside of a closure now making it harder to get at. Also, this means getConfig, setConfig, etc would need to be updated to pass in the closure reference to that state so they could interact with it.

import { newConfig } from './config'; const config = newConfig(); $$PREBID_GLOBAL$$.getConfig = config.getConfig $$PREBID_GLOBAL$$.setConfig = config.setConfig

Wouldn't each of the tests have to do this as well in order to reset global Prebid.js state? I think i'd be a little more straight forward to have a resetConfig() that clears any config state, in this case config and listeners.

snapwich · 2017-07-27T17:08:24Z

src/config.js

+ * // subscribe to only 'logging' changes
+ * subscribe('logging', (config) => console.log('logging set:', config));
+ */
+export function subscribe(topic, listener) {


I like how this subscribe is implemented now. 👍

However, I think it might be nice to merge its functionality with getConfig for a few reasons. For one, I think that getConfig is a more descriptive name, especially if we export this on the public API (which I think we should). Also, it really is fulfilling the same purpose as getConfig; it's just implemented as a push rather than a pull.

I propose we merge subscribe into getConfig. if getConfig is called with no arguments or string, behave as getConfig does now, if it's called with a function or a string and a function, then it behaves as subscribe behaves now.

dbemiller · 2017-08-02T16:48:44Z

src/prebid.js

+ * Get Prebid config options
+ * @param {object} options
+ */
+$$PREBID_GLOBAL$$.getConfig = config.getConfig;


This doesn't need to be on the global API, does it?

I was the one who suggested this. I think it's a small thing but makes Prebid.js more extensible by third-party libraries. So overall I think it's a small net-gain for a one-liner.

dbemiller · 2017-08-02T16:49:52Z

src/prebid.js

+
+/**
+ * Set Prebid config options
+ * @param {object} options


More specific JSDocs would be super helpful here.

I understand we should probably merge this PR to unblock people, though... so can we make it a priority shortly after? It improves the developer experience a ton.

snapwich

LGTM

* Add setConfig API method * Use getter/setter methods in config module to manage configuration * Use getter/setter on object props to manage legacy api access * Convert bidderTimeout * Add and use getConfig * Convert publisherDomain * Convert cookieSyncDelay * priceGranularity calls setPriceGranularity * Convert enableSendAllBids * s2sConfig calls setS2SConfig * bidderSequence calls setBidderSequence * Add subscribe function * Enable subscribing to specific configuration changes * Simplify function exposure * Add missing pbjs * Merge subscribe into getConfig and return unsubscribe function * Expose getConfig on global * Reset config in tests * Refactor

matthewlane added the in progress label Jun 26, 2017

matthewlane force-pushed the set-config-api branch 2 times, most recently from 6369692 to 7f7a470 Compare June 26, 2017 22:12

protonate self-requested a review June 27, 2017 13:42

matthewlane added on hold and removed in progress labels Jul 12, 2017

matthewlane removed the on hold label Jul 19, 2017

matthewlane force-pushed the set-config-api branch 2 times, most recently from 92dcc5b to e53a2de Compare July 21, 2017 23:23

snapwich requested changes Jul 24, 2017

View reviewed changes

matthewlane force-pushed the set-config-api branch from b71e1d7 to a190c30 Compare July 24, 2017 22:57

matthewlane added in progress needs review and removed in progress labels Jul 24, 2017

snapwich reviewed Jul 26, 2017

View reviewed changes

dbemiller reviewed Jul 27, 2017

View reviewed changes

snapwich reviewed Jul 27, 2017

View reviewed changes

matthewlane added in progress and removed needs review labels Jul 27, 2017

matthewlane force-pushed the set-config-api branch from 421b3b5 to dd2849b Compare July 28, 2017 00:00

snapwich previously approved these changes Jul 28, 2017

View reviewed changes

dbemiller assigned mkendall07 Jul 31, 2017

Add setConfig API method

260296f

matthewlane added 13 commits August 1, 2017 14:10

Convert publisherDomain

ec9aa7e

Convert cookieSyncDelay

0904e24

priceGranularity calls setPriceGranularity

a5482e7

Convert enableSendAllBids

1230058

s2sConfig calls setS2SConfig

5bab372

bidderSequence calls setBidderSequence

cd52226

Add subscribe function

b09f3f0

Enable subscribing to specific configuration changes

bfe2db8

Simplify function exposure

1b71a4d

Add missing pbjs

24197a8

Merge subscribe into getConfig and return unsubscribe function

076e6c5

Expose getConfig on global

04dc27d

Reset config in tests

0734fe7

matthewlane dismissed snapwich’s stale review via 6582de6 August 1, 2017 23:06

matthewlane force-pushed the set-config-api branch 4 times, most recently from b0ae9dd to de4831d Compare August 2, 2017 16:02

Refactor

9cbeeb3

matthewlane force-pushed the set-config-api branch from de4831d to 9cbeeb3 Compare August 2, 2017 16:36

dbemiller approved these changes Aug 2, 2017

View reviewed changes

dbemiller added LGTM and removed in progress labels Aug 2, 2017

snapwich approved these changes Aug 2, 2017

View reviewed changes

mkendall07 approved these changes Aug 4, 2017

View reviewed changes

mkendall07 merged commit d5a200c into master Aug 4, 2017

matthewlane deleted the set-config-api branch August 7, 2017 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setConfig method #1325

Add setConfig method #1325

matthewlane commented Jun 26, 2017 •

edited

Loading

dbemiller commented Jun 27, 2017

mkendall07 commented Jun 27, 2017 •

edited

Loading

dbemiller commented Jun 27, 2017 •

edited

Loading

dbemiller commented Jun 28, 2017 •

edited

Loading

snapwich commented Jun 28, 2017

dbemiller commented Jun 28, 2017 •

edited

Loading

mkendall07 commented Jul 17, 2017

snapwich Jul 24, 2017

matthewlane Jul 24, 2017

snapwich Jul 24, 2017

matthewlane Jul 25, 2017

snapwich Jul 26, 2017 •

edited

Loading

dbemiller Jul 27, 2017

matthewlane Jul 28, 2017

dbemiller Jul 27, 2017

dbemiller Jul 27, 2017

snapwich Jul 27, 2017

matthewlane Jul 27, 2017

dbemiller Jul 27, 2017

matthewlane Jul 28, 2017

dbemiller Jul 27, 2017

snapwich Jul 27, 2017

dbemiller Jul 27, 2017

dbemiller Jul 27, 2017

snapwich Jul 27, 2017

snapwich Jul 27, 2017

dbemiller Aug 2, 2017

snapwich Aug 2, 2017

dbemiller Aug 2, 2017

snapwich left a comment

Add setConfig method #1325

Add setConfig method #1325

Conversation

matthewlane commented Jun 26, 2017 • edited Loading

Type of change

Description of change

Other information

dbemiller commented Jun 27, 2017

mkendall07 commented Jun 27, 2017 • edited Loading

dbemiller commented Jun 27, 2017 • edited Loading

dbemiller commented Jun 28, 2017 • edited Loading

snapwich commented Jun 28, 2017

dbemiller commented Jun 28, 2017 • edited Loading

mkendall07 commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapwich Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapwich left a comment

Choose a reason for hiding this comment

matthewlane commented Jun 26, 2017 •

edited

Loading

mkendall07 commented Jun 27, 2017 •

edited

Loading

dbemiller commented Jun 27, 2017 •

edited

Loading

dbemiller commented Jun 28, 2017 •

edited

Loading

dbemiller commented Jun 28, 2017 •

edited

Loading

snapwich Jul 26, 2017 •

edited

Loading