Skip to content

Commit

Permalink
New hooks API (replaces monkey-patching for currency) (prebid#1683)
Browse files Browse the repository at this point in the history
* added hook module to prebid core that allows extension of arbitrary functions

* remove unused dependency tiny-queue

* change PluginFunction to HookedFunction

* more hook documentation fixes

* allow context for hooked functions

* added tests for context

* remove withContext, just use bind

* fix in hooks so asyncSeries keeps proper bound context
  • Loading branch information
snapwich authored and mattpr committed Oct 31, 2017
1 parent baa4d03 commit 5de89f3
Show file tree
Hide file tree
Showing 5 changed files with 283 additions and 78 deletions.
67 changes: 28 additions & 39 deletions modules/currency.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ var conversionCache = {};
var currencyRatesLoaded = false;
var adServerCurrency = 'USD';

// Used as reference to the original bidmanager.addBidResponse
var originalBidResponse;

export var currencySupportEnabled = false;
export var currencyRates = {};
var bidderCurrencyDefault = {};
Expand Down Expand Up @@ -81,12 +78,9 @@ function initCurrency(url) {
conversionCache = {};
currencySupportEnabled = true;

if (!originalBidResponse) {
utils.logInfo('Installing addBidResponse decorator for currency module', arguments);
utils.logInfo('Installing addBidResponse decorator for currency module', arguments);

originalBidResponse = bidmanager.addBidResponse;
bidmanager.addBidResponse = addBidResponseDecorator(bidmanager.addBidResponse);
}
bidmanager.addBidResponse.addHook(addBidResponseHook, 100);

if (!currencyRates.conversions) {
ajax(url, function (response) {
Expand All @@ -103,12 +97,9 @@ function initCurrency(url) {
}

function resetCurrency() {
if (originalBidResponse) {
utils.logInfo('Uninstalling addBidResponse decorator for currency module', arguments);
utils.logInfo('Uninstalling addBidResponse decorator for currency module', arguments);

bidmanager.addBidResponse = originalBidResponse;
originalBidResponse = undefined;
}
bidmanager.addBidResponse.removeHook(addBidResponseHook);

adServerCurrency = 'USD';
conversionCache = {};
Expand All @@ -118,37 +109,35 @@ function resetCurrency() {
bidderCurrencyDefault = {};
}

export function addBidResponseDecorator(fn) {
return function(adUnitCode, bid) {
if (!bid) {
return fn.apply(this, arguments); // if no bid, call original and let it display warnings
}
export function addBidResponseHook(adUnitCode, bid, fn) {
if (!bid) {
return fn.apply(this, arguments); // if no bid, call original and let it display warnings
}

let bidder = bid.bidderCode || bid.bidder;
if (bidderCurrencyDefault[bidder]) {
let currencyDefault = bidderCurrencyDefault[bidder];
if (bid.currency && currencyDefault !== bid.currency) {
utils.logWarn(`Currency default '${bidder}: ${currencyDefault}' ignored. adapter specified '${bid.currency}'`);
} else {
bid.currency = currencyDefault;
}
let bidder = bid.bidderCode || bid.bidder;
if (bidderCurrencyDefault[bidder]) {
let currencyDefault = bidderCurrencyDefault[bidder];
if (bid.currency && currencyDefault !== bid.currency) {
utils.logWarn(`Currency default '${bidder}: ${currencyDefault}' ignored. adapter specified '${bid.currency}'`);
} else {
bid.currency = currencyDefault;
}
}

// default to USD if currency not set
if (!bid.currency) {
utils.logWarn('Currency not specified on bid. Defaulted to "USD"');
bid.currency = 'USD';
}
// default to USD if currency not set
if (!bid.currency) {
utils.logWarn('Currency not specified on bid. Defaulted to "USD"');
bid.currency = 'USD';
}

// execute immediately if the bid is already in the desired currency
if (bid.currency === adServerCurrency) {
return fn.apply(this, arguments);
}
// execute immediately if the bid is already in the desired currency
if (bid.currency === adServerCurrency) {
return fn.apply(this, arguments);
}

bidResponseQueue.push(wrapFunction(fn, this, arguments));
if (!currencySupportEnabled || currencyRatesLoaded) {
processBidResponseQueue();
}
bidResponseQueue.push(wrapFunction(fn, this, arguments));
if (!currencySupportEnabled || currencyRatesLoaded) {
processBidResponseQueue();
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/bidmanager.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { isValidVideoBid } from './video';
import { getCacheUrl, store } from './videoCache';
import { Renderer } from 'src/Renderer';
import { config } from 'src/config';
import { createHook } from 'src/hook';

var CONSTANTS = require('./constants.json');
var AUCTION_END = CONSTANTS.EVENTS.AUCTION_END;
Expand Down Expand Up @@ -82,7 +83,7 @@ exports.bidsBackAll = function () {
/*
* This function should be called to by the bidder adapter to register a bid response
*/
exports.addBidResponse = function (adUnitCode, bid) {
exports.addBidResponse = createHook('asyncSeries', function (adUnitCode, bid) {
if (isValid()) {
prepareBidForAuction();

Expand Down Expand Up @@ -250,7 +251,7 @@ exports.addBidResponse = function (adUnitCode, bid) {
doCallbacksIfNeeded();
}
}
};
});

function getKeyValueTargetingPairs(bidderCode, custBidObj) {
var keyValues = {};
Expand Down
78 changes: 78 additions & 0 deletions src/hook.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@

/**
* @typedef {function} HookedFunction
* @property {function(function(), [number])} addHook A method that takes a new function to attach as a hook
* to the HookedFunction
* @property {function(function())} removeHook A method to remove attached hooks
*/

/**
* A map of global hook methods to allow easy extension of hooked functions that are intended to be extended globally
* @type {{}}
*/
export const hooks = {};

/**
* A utility function for allowing a regular function to be extensible with additional hook functions
* @param {string} type The method for applying all attached hooks when this hooked function is called
* @param {function()} fn The function to make hookable
* @param {string} hookName If provided this allows you to register a name for a global hook to have easy access to
* the addHook and removeHook methods for that hook (which are usually accessed as methods on the function itself)
* @returns {HookedFunction} A new function that implements the HookedFunction interface
*/
export function createHook(type, fn, hookName) {
let _hooks = [{fn, priority: 0}];

let types = {
sync: function(...args) {
_hooks.forEach(hook => {
hook.fn.apply(this, args);
});
},
asyncSeries: function(...args) {
let curr = 0;

const asyncSeriesNext = (...args) => {
let hook = _hooks[++curr];
if (typeof hook === 'object' && typeof hook.fn === 'function') {
return hook.fn.apply(this, args.concat(asyncSeriesNext))
}
};

return _hooks[curr].fn.apply(this, args.concat(asyncSeriesNext));
}
};

if (!types[type]) {
throw 'invalid hook type';
}

let methods = {
addHook: function(fn, priority = 10) {
if (typeof fn === 'function') {
_hooks.push({
fn,
priority: priority
});

_hooks.sort((a, b) => b.priority - a.priority);
}
},
removeHook: function(removeFn) {
_hooks = _hooks.filter(hook => hook.fn === fn || hook.fn !== removeFn);
}
};

if (typeof hookName === 'string') {
hooks[hookName] = methods;
}

function hookedFn(...args) {
if (_hooks.length === 0) {
return fn.apply(this, args);
}
return types[type].apply(this, args);
}

return Object.assign(hookedFn, methods);
}
151 changes: 151 additions & 0 deletions test/spec/hook_spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@

import { expect } from 'chai';
import { createHook, hooks } from 'src/hook';

describe('the hook module', () => {
let sandbox;

beforeEach(() => {
sandbox = sinon.sandbox.create();
});

afterEach(() => {
sandbox.restore();
});

it('should call all sync hooks attached to a function', () => {
let called = [];
let calledWith;

let testFn = () => {
called.push(testFn);
};
let testHook = (...args) => {
called.push(testHook);
calledWith = args;
};
let testHook2 = () => {
called.push(testHook2);
};
let testHook3 = () => {
called.push(testHook3);
};

let hookedTestFn = createHook('sync', testFn, 'testHook');

hookedTestFn.addHook(testHook, 50);
hookedTestFn.addHook(testHook2, 100);

// make sure global test hooks work as well (with default priority)
hooks['testHook'].addHook(testHook3);

hookedTestFn(1, 2, 3);

expect(called).to.deep.equal([
testHook2,
testHook,
testHook3,
testFn
]);

expect(calledWith).to.deep.equal([1, 2, 3]);

called = [];

hookedTestFn.removeHook(testHook);
hooks['testHook'].removeHook(testHook3);

hookedTestFn(1, 2, 3);

expect(called).to.deep.equal([
testHook2,
testFn
]);
});

it('should allow context to be passed to hooks, but keep bound contexts', () => {
let context;
let fn = function() {
context = this;
};

let boundContext = {};
let calledBoundContext;
let hook = function() {
calledBoundContext = this;
}.bind(boundContext);

let hookFn = createHook('sync', fn);
hookFn.addHook(hook);

let newContext = {};
hookFn.bind(newContext)();

expect(context).to.equal(newContext);
expect(calledBoundContext).to.equal(boundContext);
});

describe('asyncSeries', () => {
it('should call function as normal if no hooks attached', () => {
let fn = sandbox.spy();
let hookFn = createHook('asyncSeries', fn);

hookFn(1);

expect(fn.calledOnce).to.equal(true);
expect(fn.firstCall.args[0]).to.equal(1);
});

it('should call hooks correctly applied in asyncSeries', () => {
let called = [];

let testFn = (called) => {
called.push(testFn);
};
let testHook = (called, next) => {
called.push(testHook);
next(called);
};
let testHook2 = (called, next) => {
called.push(testHook2);
next(called);
};

let hookedTestFn = createHook('asyncSeries', testFn);
hookedTestFn.addHook(testHook);
hookedTestFn.addHook(testHook2);

hookedTestFn(called);

expect(called).to.deep.equal([
testHook,
testHook2,
testFn
]);
});

it('should allow context to be passed to hooks, but keep bound contexts', () => {
let context;
let fn = function() {
context = this;
};

let boundContext1 = {};
let calledBoundContext1;
let hook1 = function(next) {
calledBoundContext1 = this;
next()
}.bind(boundContext1);

let hookFn = createHook('asyncSeries', fn);
hookFn.addHook(hook1);

let newContext = {};
hookFn = hookFn.bind(newContext);
hookFn();

expect(context).to.equal(newContext);
expect(calledBoundContext1).to.equal(boundContext1);
});
});
});
Loading

0 comments on commit 5de89f3

Please sign in to comment.