Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Count bids with function on adapter if present #773

Closed
wants to merge 4 commits into from

Conversation

protonate
Copy link
Collaborator

Type of change

  • Other

Description of change

We count bids expected so we know when to invoke callbacks for a placement and for the overall request. To support counting bids in different ways this PR will use a function countBids, if available, in place of the default behavior.

};
};

function _countBids(bid) {
const bidCount = bid.sizes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have this unused variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cruft, will remove.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few suggestions.

@@ -40,34 +41,41 @@ function getBidders(bid) {
return bid.bidder;
}

function add(a, b) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's put into utils


/**
* Bidders handle multiple requests and multiple sizes in different ways.
* A Bidder Adapter can export a `countBids` function if this is anything
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we call this "countExpectedBids" ?

@mkendall07
Copy link
Member

I restarted travis and it still is failing with Uncaught TypeError: object is not a function at /home/travis/build/prebid/Prebid.js/test/spec/adapters/aardvark_spec.js:10281

Seems random but maybe not...

@protonate
Copy link
Collaborator Author

@mkendall07 review notes addressed

* Returns the adaptermanager.bidderRegistry
* @returns {*}
*/
$$PREBID_GLOBAL$$.getBidderRegistry = function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this added to the global? We shouldn't need to expose this. If we need a means to store objects let's put it into a separate file that does not have global access.

@protonate
Copy link
Collaborator Author

The current IX adapter bid count workaround, while inelegant, is working. I've put this is on hold for the time being. We should determine the best way to either support custom bid counting or conforming to a standard way of doing this.

cc: @prebid/indexexchange @indexexchange

@protonate
Copy link
Collaborator Author

Stale PR, closing, if there is further interest here please reopen.

@protonate protonate closed this Feb 1, 2017
@protonate protonate deleted the improvement/expected-bids-custom-transform branch May 5, 2017 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants