-
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. bugfix for singleRequest config regression #9050
Rubicon. bugfix for singleRequest config regression #9050
Conversation
* fix rubicon singleRequest config regression prebid#9049 * manually kick off testing Co-authored-by: Chris Huie <[email protected]>
* fix rubicon singleRequest config regression prebid#9049 * manually kick off testing Co-authored-by: Chris Huie <[email protected]>
Not sure how this got in and merged without someone from Magnite / Rubicon reviewing or commenting at least. But I fail to see how this was not working? We do keep an ongoing internal cache of any update made to the Unless you are using prebid in a different way, where you load prebid core first, then run all your que block setConfigs, and then initialize other modules. Because the que block should be executed AFTER modules are loaded. Including our bid adapter. And since that is the case, our module is initialized and should register to the config BEFORE anything in the queue block does. You can test this by putting breakpoints in your first que block function and inside of a module. The code before the change you implemented: let rubiConf = {};
// we are saving these as global to this module so that if a pub accidentally overwrites the entire
// rubicon object, then we do not lose other data
config.getConfig('rubicon', config => {
mergeDeep(rubiConf, config.rubicon);
});
// ..... and in bidRequest time
if (rubiConf.singleRequest !== true) { This saves any setConfig rubicon call into rubiConf. and this if statement will evaluate to FALSE and hit the else statement if you do your example: pbjs.que.push(() => {
pbjs.setConfig({
rubicon: {
singleRequest: true
}
});
}); It 100% does run singleRequest auctions and keeps the state of what the above setConfig implements. Since que blocks are executed after modules have been initialized. Here are some screenshots which prove this (I added info logs to show how this works) And we see only one call to Rubicon Fastlane endpoint: And here is the reverse where we turn singleRequest off In fact this PR introduces what I consider a bug, and the whole reason we added this logic in the commit you reference as introducing the bug. With the change implemented by this PR the following would turn OFF single request auctions pbjs.que.push(() => {
pbjs.setConfig({
rubicon: {
singleRequest: true
}
});
pbjs.setConfig({
rubicon: {
netRevenue: true
}
});
}); Which is obviously NOT intended. These cases where multiple setConfigs for our This was before the adoption of Please, if you are going to propose a PR for an adapter, the maintainers should be tagged first and given some time to comment and review the PR. If I am mistaken and you can provide an example test page where my change would not work please share. But from my testing it works just fine. Here is a jsfiddle which points to Prebid 7.18.0 which is the version before this PR was merged. https://jsfiddle.net/0khqzt15/12/ I am planning to revert this PR back to the old implementation. I am not aware that there is a use case where some This line of code should handle if this is a valid use case. let rubiConf = pbjs.getConfig('rubicon') || {}; |
Here is the PR #10332 This PR reverts back to the old method I had set, as well as solve for any potential use case where a |
* revert PR #9050 * revert pr 9050
…d#10332) * revert PR prebid#9050 * revert pr 9050
Type of change
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
please see #9049