New hooks API (replaces monkey-patching for currency)

[snapwich]

Type of change

• Feature
• Refactoring (no functional changes, no api changes)

Description of change

This pull-request provides a new API for adding hooks into the Prebid core functionality. By wrapping code paths with createHook, Prebid.js modules (and possibly external code, if we decide to expose hooks) can extend the functionality of core code through various models provided in hook.js. It follows in the spirit of https://github.com/webpack/tapable (the library webpack uses for adding extensibility). However, rather than adding extensibility for class methods through mixins (like tapable), I opted for adding extensibility to plain function since much of Prebid core is functional in nature.

So far the only extensibility options for hooks that I added are sync and asyncSeries, but other could be added if needed as seen within the tapable library's readme.

This replaces the monkey-patching that currently takes place in the currency module. Once merged, the 1.0 branch can be updated to put the appropriate hook in place for currency to still function properly.

The hook API in this pull-request being used for bidmanager.addBidResponse in pre 1.0 is like the following:

// bidmanager.js
import { createHook } from 'src/hook.js';

exports.addBidResponse = createHook('asyncSeries', function(adUnitCode, bid) {
  // add bid response stuff
});

This allows any module that wants to extend bidmanager.addBidResponse functionality to import the method and add a hook.

// some module
import { addBidResponse } from 'src/bidmanager.js';
addBidResponse.addHook(function(adUnitCode, bid, next) {
  // modify bid or something
  next(adUnitCode, bid); // continue execution of further hooks, or finally call addBidResponse if this is only hook
}, 50); // `addHook` allows for optional parameter specifying priority.  Higher priority hooks will run first

For making this compatible with 1.0 we need only to wrap createHook around whatever method we want extended (I think a new addBidResponse w/o the bidmanager) and expose the hook in a way that modules can get access to them . Either export the function in the new auctionManager (or wherever) or provide a global hook name as a third parameter to createHook.

Example:

// auction.js
adaptermanager.callBids(_adUnits, bidRequests, createHook('asyncSeries', addBidResponse.bind(this), 'addBidResponse'), done.bind(this)); // global hook named 'addBidResponse' created

// some module
import { hooks } from 'src/hook.js';

hooks['addBidResponse'].addHook(function() { ... });

With this new API we could hopefully start creating more hook-points in Prebid.js core which allows for more code to be moved into modules for optional inclusion.

[dbemiller]

This is much more elegant than monkey-patching... but I'm not sure it solves the 1.0 problem.

The core problem in 1.0 is that the addBidResponse function is no longer global. Instead, each auction instance makes its own addBidResponse.

This means that the currency module (which is global) can't get a reference to it, so it would be impossible to call addHook using the method reference directly.

I think the global API (hooks[name].addHook() and hooks[name].createHook()) has a similar, related problem.

The first auction runs createHook, which makes a hooks['addBidResponse']. The currency module adds its decorator. Then a second auction gets created, which sets hooks['addBidResponse'] to a new object. The currency module would only get applied once.

I haven't looked into the 1.0 branch closely enough yet... but I think there's also a race condition here. If a pub does something like:

pbjs.setConfig(currencyStuff);
setTimeout(function() {
  pbjs.callBids();
}, 5000)

then there's a good chance that initCurrency will run before createHook ever gets created. In that case, hooks['addBidResponse'].addHook() will end in an undefined reference error.

[snapwich]

So I attempted a few methods to solve the auction instance problems w/ 1.0 such as allowing callbacks to hooks and having addBidResponse be one, that way it can keep its closure context and we could have a global hook.

However, the method that I think makes the most sense is fixing addBidResponse. In its current state it requires a lot of context to operate, and that context is grabbed from its outside scope creating a closure with a bunch of implicit dependencies. It's very hard to debug and follow. I suggest we make instance dependencies explicit through use of this, such as this._bidderRequest.find as opposed to _bidderRequest.find. This allows us to set the appropriate instance context for each callback passed to the adapters. This would also allow us to move addBidResponse to a more global scope to create a hook.

I updated this pull-request to be more accepting of context passing and binding for hook functions.

The changes to auction.js would look similar to this:

// outer module scope
export const addBidResponse = createHook('asyncSeries', function addBidResponse(adUnitCode, bid) {
  ...
  function addBidToAuction() {
    events.emit(CONSTANTS.EVENTS.BID_RESPONSE, bid);
    this.addBidReceived(bid); // instance functions and properties referenced by this
  }
})

// callBids auction instance method could go back to the following
function callBids() {
  ...

  adaptermanager.callBids(_adUnits, bidRequests, addBidResponse.bind(this), done.bind(this));
};

Basically we'll be treating addBidResponse more like a public prototype method (even though we don't use classes) than an inner private closure function, which I think is ideal. Thoughts?

[dbemiller]

I think this will work, as a general strategy... but there are a few bugs which should probably be fixed before merging.

[dbemiller]

I'm not sure exactly what the issue(s) are... but I uncovered some weird bugs in asyncSeries hooks.

The following test passes, as expected:

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

      hookFn();

      expect(calledBoundContext1).to.equal(boundContext1);
    });

However, it begins failing if you add a second hook, like so:

   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 boundContext2 = {};
      let calledBoundContext2;
      let hook2 = function(next) {
        calledBoundContext2 = this;
        next()
      }.bind(boundContext2);

      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);
      expect(calledBoundContext2).to.equal(boundContext2);
    });

It also fails with a single hook if the hookable function is bound:

    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);
    });

The error messages aren't very helpful... just TypeError: Cannot read property 'replace' of undefined, with no line number.

[snapwich]

The first test case you list I think is failing because you never added the second hook. I think adding hookFn.addHook(hook2); should fix it.

The second test case is a bug. I wasn't properly maintaining the this reference in the asyncSeriesNext function. I updated the code to use a fat-arrow function to keep the proper this and updated the test w/ your use case and it seems to be passing now. Thanks.

[dbemiller]

is there any value in setting a priority when no hooks compete yet?

[snapwich]

Yes. If others add hooks later this one will still run first. I specifically wanted currency running first as it buffers responses (if the currency file isn't back yet).

[snapwich]

updated