-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
rubicon converted to bidderFactory #1624
rubicon converted to bidderFactory #1624
Conversation
return bids; | ||
}, | ||
getUserSyncs: function() { | ||
if (!hasSynced) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand the business use-cases behind this, but... is there any reason why someone would want to sync pixels more than once?
If not, could it be pulled down into the user sync module? Goal being to reduce the number of things that adapters need to worry about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some use for that; although it'd have to be a configuration option I think in the case that a bidder wanted to fire multiple syncs to the same endpoint for whatever reason.
It's a good suggestion, maybe we should open that as a separate issue? We are hoping to get these changes in the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's fine for now. There's a release later today, and it's definitely mergeable.
Be careful leaving it for too long, though. Adapters will look towards these first few files as an example for how to build their own... so any tech debt will accrue interest very quickly over the next few months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aside from a possible change based on the comment from @dbemiller, looks good.
….30.0 to aolgithub-master * commit '5a8d2bf93ee15071a78e24ac976103cacf3c6021': (35 commits) Added changelog entry. Prebid 0.30.1 Release Remove undefined variable usage (prebid#1662) fixes bug for IE when invalid value passed to parse (prebid#1657) Aliasbidder fix (prebid#1652) prebidAdapter secure support (prebid#1655) Increment pre version Prebid 0.30.0 Release Add native param support to mediaTypes (prebid#1625) PulsePoint Lite adpater changes (prebid#1630) Appnexus ast unittest updates (prebid#1654) Support aspect ratio specification for native images (prebid#1634) Revert changes for switch between client side and server side. (prebid#1653) rubicon converted to bidderFactory (prebid#1624) Add JSDoc for `pbjs.getAllWinningBids` (prebid#1566) Add ignore-loader to handle .md files (prebid#1646) fixed PBS cookie syncs (prebid#1637) Add placementId request param to Yieldmo bid adapter (prebid#1632) Adxcg analytics adapter (prebid#1599) Add publisher sub-id support to the Criteo adapter (prebid#1629) ...
Type of change
Description of change
An updated rubicon adapter that uses the new bidderFactory. Also added a few more properties to the
Bid
type definition within the bidderFactory.