Prebid 8: add bidResponse before emitting the event BID_RESPONSE

[olafbuitelaar]

Type of change

• [ x] Bugfix

• Feature

• New bidder adapter

• Code style update (formatting, local variables)

• Refactoring (no functional changes, no api changes)

• Build related changes

• CI related changes

• Does this change affect user-facing APIs or examples documented on http://prebid.org?

• Other

Description of change

Right now when an auction instance receives a bid, it triggers the event BID_RESPONSE and then add's it to the internal collection _bidsReceived. This becomes problematic when you depend on the bid already available in the auction. I think the order should be reversed, so that it first stores the bid, and then triggers the BID_RESPONSE event. I don't expect this to break existing functionalities.

Other information

original changes: #2067

[dgirardi]

This becomes problematic when you depend on the bid already available in the auction

this change would be problematic when you depend on the bid not being already in the auction, which is IMO more likely, simply because that's how it worked until now.

To be clear I'm not against this but I'm not confident it's not breaking (we have no way to tell what people do in their event handlers). What is your use case?

[olafbuitelaar]

I agree, that's why i put it mildly, it was just based on the fact all tests pass, and that the remaining functionality is private. But i've no way to know for sure of course. My use case, is that on the bid-response event, i would want to be able to render the ad directly, which currently fails, because renderAd cannot find it. What would be the use-case for depending the ad not the be there, when the event is triggered? judging on the existing unit test it seems to be mainly used by analytical modules. And maybe more generally it would make sense the event is only triggered, when the internal state reflects the actual situation, unless you want to know for sure the bid received isn't already received before, which seems unlikely, when this is a valid case we could also add a 2nd param to the event, indicating whether or not it was already in the _bidsReceived

[dgirardi]

I agree that this order makes more sense, and I can't think of a good reason why you'd want to keep the current one. But I'm still worried about changing the public API - what do you (and @patmmccann) think about putting this in version 8 out of an abundance of caution? And in the meanwhile,

render the ad directly, which currently fails, because renderAd cannot find it

I think you should be able to work around it with a 0-delay timeout, e.g. changing pbjs.onEvent('bidResponse', handler) to pbjs.onEvent('bidResponse', (...args) => setTimeout(() => handler(...args)))

[olafbuitelaar]

Yeah working around it, shouldn't be a problem, thanks for the suggestion, but i think ideally this shouldn't be necessary.

[patmmccann]

We're working on some stuff to render ads directly, eg in the event of a gam outage, asking @mmoschovas to take a look as he can best comment on if this might affect someone attempting a similar effort.

Fwiw this seems like a great idea to me.