From 8420558d2e565b1951713b9a90a940bfb9d0b248 Mon Sep 17 00:00:00 2001 From: Rich Snapp Date: Mon, 30 Oct 2017 08:53:18 -0600 Subject: [PATCH] New hooks API (replaces monkey-patching for currency) (#1683) * 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 --- modules/currency.js | 67 ++++++------- src/bidmanager.js | 5 +- src/hook.js | 78 +++++++++++++++ test/spec/hook_spec.js | 151 +++++++++++++++++++++++++++++ test/spec/modules/currency_spec.js | 60 +++++------- 5 files changed, 283 insertions(+), 78 deletions(-) create mode 100644 src/hook.js create mode 100644 test/spec/hook_spec.js diff --git a/modules/currency.js b/modules/currency.js index 1d0286ed569..19e7d2903f4 100644 --- a/modules/currency.js +++ b/modules/currency.js @@ -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 = {}; @@ -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) { @@ -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 = {}; @@ -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(); } } diff --git a/src/bidmanager.js b/src/bidmanager.js index c12cc4828e6..abd8fea135d 100644 --- a/src/bidmanager.js +++ b/src/bidmanager.js @@ -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; @@ -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(); @@ -250,7 +251,7 @@ exports.addBidResponse = function (adUnitCode, bid) { doCallbacksIfNeeded(); } } -}; +}); function getKeyValueTargetingPairs(bidderCode, custBidObj) { var keyValues = {}; diff --git a/src/hook.js b/src/hook.js new file mode 100644 index 00000000000..5ba1d4b9bbf --- /dev/null +++ b/src/hook.js @@ -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); +} diff --git a/test/spec/hook_spec.js b/test/spec/hook_spec.js new file mode 100644 index 00000000000..1fab4ecd1b7 --- /dev/null +++ b/test/spec/hook_spec.js @@ -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); + }); + }); +}); diff --git a/test/spec/modules/currency_spec.js b/test/spec/modules/currency_spec.js index 937e6a084e4..06faa5665c9 100644 --- a/test/spec/modules/currency_spec.js +++ b/test/spec/modules/currency_spec.js @@ -5,7 +5,7 @@ import { import { setConfig, - addBidResponseDecorator, + addBidResponseHook, currencySupportEnabled, currencyRates @@ -46,10 +46,6 @@ describe('currency', function () { var bid = { cpm: 1, bidder: 'rubicon' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { - innerBid = bid; - }); - setConfig({ adServerCurrency: 'GBP', bidderCurrencyDefault: { @@ -57,7 +53,9 @@ describe('currency', function () { } }); - wrappedAddBidResponseFn('elementId', bid); + addBidResponseHook('elementId', bid, function(adCodeId, bid) { + innerBid = bid; + }); expect(innerBid.currency).to.equal('GBP') }); @@ -68,10 +66,6 @@ describe('currency', function () { var bid = { cpm: 1, currency: 'JPY', bidder: 'rubicon' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { - innerBid = bid; - }); - setConfig({ adServerCurrency: 'JPY', bidderCurrencyDefault: { @@ -79,7 +73,9 @@ describe('currency', function () { } }); - wrappedAddBidResponseFn('elementId', bid); + addBidResponseHook('elementId', bid, function(adCodeId, bid) { + innerBid = bid; + }); expect(innerBid.currency).to.equal('JPY') }); @@ -97,12 +93,10 @@ describe('currency', function () { var bid = { cpm: 100, currency: 'JPY', bidder: 'rubicon' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); - expect(innerBid.cpm).to.equal('1.0000'); }); }); @@ -113,14 +107,15 @@ describe('currency', function () { fakeCurrencyFileServer.respondWith(JSON.stringify(getCurrencyRates())); - var marker = false; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { - marker = true; - }); var bid = { 'cpm': 1, 'currency': 'USD' }; setConfig({ 'adServerCurrency': 'JPY' }); - wrappedAddBidResponseFn('elementId', bid); + + var marker = false; + addBidResponseHook('elementId', bid, function() { + marker = true; + }); + expect(marker).to.equal(false); fakeCurrencyFileServer.respond(); @@ -133,10 +128,9 @@ describe('currency', function () { setConfig({}); var bid = { 'cpm': 1, 'currency': 'USD' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(innerBid.cpm).to.equal(1); }); @@ -144,10 +138,9 @@ describe('currency', function () { setConfig({}); var bid = { 'cpm': 1, 'currency': 'GBP' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(innerBid.statusMessage).to.equal('Bid returned empty or error response'); }); @@ -157,10 +150,9 @@ describe('currency', function () { }); var bid = { 'cpm': 1, 'currency': 'USD' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(bid).to.equal(innerBid); }); @@ -170,10 +162,9 @@ describe('currency', function () { fakeCurrencyFileServer.respond(); var bid = { 'cpm': 1, 'currency': 'ABC' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(innerBid.statusMessage).to.equal('Bid returned empty or error response'); }); @@ -183,10 +174,9 @@ describe('currency', function () { fakeCurrencyFileServer.respond(); var bid = { 'cpm': 1, 'currency': 'GBP' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(innerBid.statusMessage).to.equal('Bid returned empty or error response'); }); @@ -196,10 +186,9 @@ describe('currency', function () { fakeCurrencyFileServer.respond(); var bid = { 'cpm': 1, 'currency': 'JPY' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(innerBid.cpm).to.equal(1); expect(innerBid.currency).to.equal('JPY'); }); @@ -210,10 +199,9 @@ describe('currency', function () { fakeCurrencyFileServer.respond(); var bid = { 'cpm': 1, 'currency': 'USD' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(innerBid.cpm).to.equal('0.7798'); expect(innerBid.currency).to.equal('GBP'); }); @@ -224,10 +212,9 @@ describe('currency', function () { fakeCurrencyFileServer.respond(); var bid = { 'cpm': 1, 'currency': 'CNY' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(innerBid.cpm).to.equal('0.1133'); expect(innerBid.currency).to.equal('GBP'); }); @@ -238,10 +225,9 @@ describe('currency', function () { fakeCurrencyFileServer.respond(); var bid = { 'cpm': 1, 'currency': 'JPY' }; var innerBid; - var wrappedAddBidResponseFn = addBidResponseDecorator(function(adCodeId, bid) { + addBidResponseHook('elementId', bid, function(adCodeId, bid) { innerBid = bid; }); - wrappedAddBidResponseFn('elementId', bid); expect(innerBid.cpm).to.equal('0.0623'); expect(innerBid.currency).to.equal('CNY'); });