Prebid core: fix exception in requestBids introduced by #9106
#9194
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 64aff9b.
|
Pr updated with an actual fix. |
dgirardi
changed the title
requestBids (#9106)"requestBids introduced by #9106
dgirardi
force-pushed
the
criteo-requestBids
branch
from
4bfb136 to
f9c72fa
Compare
dgirardi
force-pushed
the
criteo-requestBids
branch
from
f9c72fa to
58a35ae
Compare
muuki88
approved these changes
Nov 3, 2022
Comment on lines
+646
to
+648
| req.defer = defer({promiseFactory: (r) => new Promise(r)}) | ||
| delegate.call(this, req); | ||
| return req.defer.promise; |
There was a problem hiding this comment.
Maybe a few comments why this is necessary would be helpful.
As someone only looking at bid adapters mostly the fun-hook framework and the custom promise implementation this code is non-obivous why it's there.
There was a problem hiding this comment.
If you're asking about the reason for fun-hooks or the custom promise:
- fun-hooks is basically a plugin system (to the author: sorry if I'm oversimplifying this). if we have 5 modules that may be loaded in a random combination and order, and all plug into the same code path, fun-hook solves the problem of orchestrating those. I can't answer why we are using that over something else.
- the custom promise -
A version of Promise that runs callbacks synchronously when it can- came about because of this discussion.
As for this code specifically, I tried clarifying it in the comments - it boils down to idiosyncrasies of fun-hooks.
There was a problem hiding this comment.
Thanks for the clarification 🤗
| @@ -1665,7 +1677,7 @@ describe('Unit: Prebid Module', function () { | |||
| sinon.assert.match(bids[bid.adUnitCode].bids[0], bid) | |||
| done(); | |||
| }); | |||
| completeAuction([bid]); | |||
| setTimeout(() => completeAuction([bid])); | |||
There was a problem hiding this comment.
also a small comment here why wrapping in setTimeout is required here
| @@ -622,15 +623,15 @@ $$PREBID_GLOBAL$$.removeAdUnit = function (adUnitCode) { | |||
| * @alias module:pbjs.requestBids | |||
| */ | |||
| $$PREBID_GLOBAL$$.requestBids = (function() { | |||
| const delegate = hook('sync', function ({ bidsBackHandler, timeout, adUnits, adUnitCodes, labels, auctionId, ttlBuffer, ortb2, metrics } = {}) { | |||
| const delegate = hook('async', function ({ bidsBackHandler, timeout, adUnits, adUnitCodes, labels, auctionId, ttlBuffer, ortb2, metrics, defer } = {}) { | |||
There was a problem hiding this comment.
When I discovered the issue a little more comments would have helped what the intention of this delegate method was or what it is supposed to do.
ChrisHuie
approved these changes
Nov 3, 2022
dgirardi
added a commit
to dgirardi/Prebid.js
that referenced
this pull request
Nov 3, 2022
…prebid#9194) * Revert "Prebid core: return a promise from `requestBids` (prebid#9106)" This reverts commit 64aff9b. * Revert "Revert "Prebid core: return a promise from `requestBids` (prebid#9106)"" This reverts commit bf5ee30. * Prebid core: fix exception in `requestBids` introduced by prebid#9106 * Add comments
1 task
JacobKlein26
pushed a commit
to nextmillenniummedia/Prebid.js
that referenced
this pull request
Feb 9, 2023
…prebid#9194) * Revert "Prebid core: return a promise from `requestBids` (prebid#9106)" This reverts commit 64aff9b. * Revert "Revert "Prebid core: return a promise from `requestBids` (prebid#9106)"" This reverts commit bf5ee30. * Prebid core: fix exception in `requestBids` introduced by prebid#9106 * Add comments
jorgeluisrocha
pushed a commit
to jwplayer/Prebid.js
that referenced
this pull request
May 23, 2023
…prebid#9194) * Revert "Prebid core: return a promise from `requestBids` (prebid#9106)" This reverts commit 64aff9b. * Revert "Revert "Prebid core: return a promise from `requestBids` (prebid#9106)"" This reverts commit bf5ee30. * Prebid core: fix exception in `requestBids` introduced by prebid#9106 * Add comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change
Description of change
Fix a bug introduced by #9106: if a module such as userId needs to delay the auction (
auctionDelay > 0), it can cause hooks to run out of order and error out.Fixes #9191