Save winning bid

[d6u]

Hi,

I'm from PubNation.
This PR will help us to properly determine the winning bidder. Listening to 'bidWon' event can sometimes cause race conditions.

Let me know if you need more information.
Thanks!

[jaiminpanchal27]

@d6u Thanks for adding winning bids.
Please write a test case for this feature.

[d6u]

@jaiminpanchal27 Thanks for looking at this. I added a test case in API spec tests. But I haven't found other places to add tests. Is this enough?

[mkendall07]

@d6u
What are the issues with listening to bidWon event? Can you describe the use case please? If you add these bids to a structure, at some point they need to be cleared out.

[d6u]

@mkendall07 definitely.

First, the bidWon event work fine for us now, but we encountered some edge cases. Our PubNation script is loaded asynchronously as a 3rd party script like Google Analytics. In order to attach bidWon event listener, the pbjs object has to exist when our code runs. There are opportunities for race conditions, e.g. if our code run earlier before pbjs is loaded on the page, it will fail. I know pbjs supports a push method but it highly depends on people's implementation, e.g. some site could load pbjs asynchronously as well. We want to eliminate that uncertainty.

Second, in order to work with bidWon event hook, our code has to do some non-trivial tasks like, 1) pull the page to see if pbjs exist, 2) attach the event listener, 3) save the bid data somewhere, and 4) match the bid data later on. If pbjs can save the winning bid, we can remove step 1 and 2. The added logic in pbjs is much less compared with our current approach.

Could talk about what's requirement for "cleared out"? I can help add that logic if necessary.

[mkendall07]

@d6u
Ok that makes sense. I only meant that the size of _winningBids will continue to grow with each refresh, and it doesn't ever get cleared out. I would recommend to garbage collect it after 10 minutes or so.

[d6u]

@mkendall07 Thanks. Is the convention here to using setTimeout or setInterval?

[mkendall07]

I actually don't want to add another timeout for this specifically as this is a bigger problem then just this one area. We will address in #477 possibly.

Can you add more unit tests to cover functionality of the method? I think once that is added this will be good to go.

[d6u]

Just added some unit test to cover both logic in renderAd and getAllWinningBids.

[mkendall07]

LGTM. Merging.