replace usePrebidCache with cache:url and remove default #1904
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
dbemiller
reviewed
Dec 4, 2017
src/config.js
Outdated
| * @property {bool} usePrebidCache True if we should use prebid-cache to store video bids before adding | ||
| * bids to the auction, and false otherwise. **NOTE** This must be true if you want to use the | ||
| * dfpAdServerVideo module. | ||
| * @property {bool} config.cacheUrl Set a url if we should use prebid-cache to store video bids before adding |
There was a problem hiding this comment.
Looks like this should be a {string}, in your current code.
Just so you're aware, we'll be building some cache features into prebid-server sometime fairly soon. If s2s is enabled, the JS code might want to take advantage of it. Since PBS and PBC will often be hosted in the same data center, you should get much better performance by asking PBS to cache things, rather than doing it client-side.
To give you some idea, this is (roughly) the API I'm planning for PBS:
{
"cache": {
"vastxml": {
"winners": true,
"deals": true,
"all": true,
"minCPM": 0.01
}
}
}
If you have feedback on this, feel free to discuss it over on prebid/prebid-server#216. For this PR, though: It might be better to make a cache: object with a url: string property. When PBS catches up, you'll be able to add support for these features without breaking the API.
There was a problem hiding this comment.
I'm cool with that. I'll update it to be cache.url
dbemiller
reviewed
Dec 4, 2017
| @@ -48,9 +49,17 @@ describe('The video cache', () => { | |||
| xhr = sinon.useFakeXMLHttpRequest(); | |||
| requests = []; | |||
| xhr.onCreate = (request) => requests.push(request); | |||
| config.setConfig({ | |||
There was a problem hiding this comment.
Why wouldn't you just use newConfig() here? Like dfpAdServerVideo_spec.js does.
It cuts down on the changes you need to make for this PR by quite a bit.
There was a problem hiding this comment.
I don't think that newConfig is working as you are intending it to. For instance, I just removed it from dfpAdServerVideo_spec.js file and the tests still pass. The actual dfpAdServerVideo.js module is operating on the config object in in config.js, not the new config object created with newConfig(), so unless you change these modules to dependency injection where you inject the config object, using newConfig doesn't do anything.
dbemiller
reviewed
Dec 4, 2017
There was a problem hiding this comment.
This may be fine, depending on how you want to do things.
I still strongly recommend newConfig over resetConfig. I assume you don't buy those arguments... but I'm certainly not going to fight about it anymore, because I'm not on the hook for maintaining it. I just mention it again in case you didn't see that the newConfig function existed, already, and thought you needed to add reset in order to make it testable.
Let me know if you plan to make any changes based on the comments I left. If not, it lgtm.
|
updated |
dbemiller
approved these changes
Dec 6, 2017
snapwich
changed the title
matthewlane
reviewed
Dec 6, 2017
| @@ -236,7 +236,7 @@ export const addBidResponse = createHook('asyncSeries', function(adUnitCode, bid | |||
|
|
|||
There was a problem hiding this comment.
@snapwich Not sure if I did something wrong when checking this branch out locally but I see one remaining reference to usePrebidCache in this file on line 155. Everything else lgtm
There was a problem hiding this comment.
No you are correct. It looks like Jaimin added a new call to usePrebidCache after the creation of this pull-request which I got with the latest merge update. Just fixed and updated, as well as removed a call to it in one of the bid adapters.
|
updated and resolved conflicts |
matthewlane
approved these changes
Dec 6, 2017
matthewlane
pushed a commit
that referenced
this pull request
Dec 7, 2017
* Concurrent auctions (#1593) * Adding timestamp function to utils * Auction manager (WIP) * WIP * Updated targeting with auction instances and added auction status * Updated appnexus and appnexusAst adapter * Added done callback * Removing bidmanager functions * Removing deprecated public api functions * remove bidmanager file * Revert "remove bidmanager file" This reverts commit 616409e. * Updated appnexus and rubicon adapters to call done callback after all responses are back * Added todo * Fixing circular dependency * filter s2s requests * Emit auction end Add getBidsRequested function * remove unused stuff from adapters * cleanup and fixes * Unit test fixes Added exclude to karma temporarily to support unit tests * added setTimeout for each auction instance * added constants closer to code * binded auction object inside done method * Adding auctionmanager_spec tests and fixes * Remove breakpoint * Moving auction to separate file and unit tests * Merged manually * Merged config api changes * Passing adunits, adunitscodes via constructor and fixing unit tests * added ajax with timeout * convert targeting module to factory pattern * Auction code refactored and unit tests * Updated bidderfactory for 1.0 changes and added unit tests * Renamed placementCode to adUnitCode * Refactor auction and update unit tests * JSDoc for auction and auctionManager * Unit test fixes after rebase * Manual merge new updates to bidmanager and its spec file into auction * Update appnexusAst spec file * Refactor * Prebid 1.0 does not require no_bid to be added * removed side effect by cloning objects * bug fix to set status on already used bid * Removing all !1.0 complaiant adapters * Adding aliases to appnexusAst adapter * Removed bidmanager from currency * Add ignore-loader to handle .md files (#1646) * Auction module refactor (#1644) * Auction moduel refactor * remove comment and global cbtimeout * Config api updates (#1633) * Add timeoutBuffer, s2sconfig, bidderSequence to config * Removing deprecated function and its unit test * fixed linting errors * Adapter/rubicon bid adapter markup (#1674) * initial take on rubiconBidAdapter markup * formatting * formatting, cont. * formatting, cont. * Prebid 1.0 Fix issue with video bid validation (#1680) * Fix issue with video bid validation * Modified tests to stub `auctionManager.getBidsRequested` instead of `getBidRequest` * Move stub to beforeEach hook * Fix lint errors * Add bidRequests param to bid validation * Auction module updated to support currency/hook (#1733) * added hook module to prebid core that allows extension of arbitrary functions * remove unused dependency tiny-queue * change PluginFunction to HookedFunction * more hook documentation fixes * Auction module updated to support currency/hook * remove unused dependency tiny-queue * change PluginFunction to HookedFunction * more hook documentation fixes * WIP * allow context for hooked functions * added tests for context * remove withContext, just use bind * fix in hooks so asyncSeries keeps proper bound context * Unit test fixes * Updated bid validation function * Fixed video unit test * updated sizeMapping to use sizeConfig and support labels (#1772) * updated sizeMapping to use sizeConfig and support labels * added new tests for labels and sizes w/ sizeConfig when making auction * made some names clearer and added type to labels for sizeMapping * make error message more descriptive in adaptermanager * remove extra line in adpatermanager * update package.json with correct version. (#1813) * Unit test fixes for IE 10 and other old browsers (#1810) * Added fix for location.origin * Fixed test case failing in IE and Safari browsers * Added utils.getOrigin method * Updated renderer to use hooks * Rename appnexusAst adapter to appnexus adapter (#1848) * Renamed appnexusAst adapter to appnexus * Updated unit test to use appnexus bidderCode * Remove completed todo comment * fixed safeframe for 1.0 (#1834) * Emit array of objects from BID_TIMEOUT event (#1824) * Emit array of objects from BID_TIMEOUT event * requestId is now auctionId * Use v4 UUID to match previous requestId format * Move function * Move public winningBids to auction (#1828) * Move public winningBids to auction * removed _winningBids * removed _winningBids * bugfix: return only new bids * Updates for PubWise Prebid 1.0 Support (#1847) * Updates for Prebid 1.0 Support * Updates for Bug Fixes and a small refactor * update JSDoc comment. Remove trailing space (#1872) tests were failing due to no trailing space eslint rule. * Prebid 1.0 adxcg analytics adapter fix for bidtimeout event (#1871) * adxcg analytics adapter for 1.0 bidtimeout event * update tests for adxcg analytics 1.0 * Prebid 1.0 prebid server (#1846) * fix adaptermanager s2sTest unit tests * fix s2s log message * remove errant comment * fixed log statement * removed seemingly unnecessary call to transformHeightWidth(adUnit); * removed legacy sizeMapping code block * initial refactor of prebidServerBidAdapter working w/o tests (cherry picked from commit 2b843d0) * add transformSizes back for prebidServer adUnits to fix request * fixed adapterManager_spec tests * added prebidServerBidAdapter tests for 1.0 * fixed lint errors * make sure addBidResponse and doneCb are stubbed for s2s calls * s2s requests now firing BID_REQUESTED event * fixed commented tests and other minor fixes * update defaults in prebidServerBidAdapter and fix doBidderSync bug * add new API for setting defaults in config for modules * Targeting updates (#1689) * Cherry pick alias bidder * Cherry pick alias bidder * Cherry pick alias bidder * Updated test case to not import adapter * targeting updates * targeting functions refactoring * Refactored functions * more refactor of function * added jsdoc and some more refactor * check bid expiry and filtering used bids * make sure we have the right version * Update version to fix invalid semver * video bug fix (#1906) * video bug fix added required params to prebidServerAdapter * Emitted auction_end and updated ttl for prebidServer * fix logging of server adapters * Manually merge bidManager code to auction module (#1905) * Unit test fix in IE for adxcgAnalytics Adapter (#1929) * IE bug fix * get only unique bidders * added log message for xhr timeout (#1928) * remove polyfill.js and remove global polyfills (#1918) * remove polyfill.js and remove global polyfills * make sure find, findIndex, and includes use core-js in tests * switch from virtual function bind core-js to explicit usage. * remove transform-function-bind babel plugin * Replace usePrebidCache with cache:url and remove default (#1904) * replace usePrebidCache with video:cacheUrl and remove default * remove newConfig in dfpAdServerVideo_spec that isn't doing anything * change video.cacheUrl to cache.url * update to cache.url in auction and remove from adapter * pulsepointLiteBidAdapter renamed to pulsepointBidAdapter (#1931) * ET-1691: Pulsepoint Analytics adapter for Prebid. (#1) * ET-1691: Adding pulsepoint analytics and tests for pulsepoint adapter * ET-1691: Adding pulsepoint analytics and tests for pulsepoint adapter * ET-1691: cleanup * ET-1691: minor * ET-1691: revert package.json change * Adding bidRequest to bidFactory.createBid method as per #509 * ET-1765: Adding support for additional params in PulsePoint adapter (#2) * ET-1850: Fixing #866 * Minor fix * Adding mandatory parameters to Bid * Pulsepoint adapter changes * readme updates * add 'x_source.tid' to rubicon requests (#1933) * Bring in pre-1.0 native-image asset adapter change (#1934) * 33Across Adapter: Removed the usage of utils library (#1917) * Removed the usage of utils library to get bidder requests during userSync since this info will now be passed into the method * Fixed extra space which cause lint to fail * Implemented user sync per code review feedback in #1917 * Minor feedback changes * Re-instated lint check for all files which was accidentally commited after testing * Prebid 1.0 server cache (#1935) * add cache stuff to video for prebid server * add description for cacheMarkup
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
This pull-request replaces the use of
usePrebidCachewithcache:{url:"<prebid cache url>"}insetConfig. It also removes the default Appnexus URL and now the cache URL must be explicitly specified. Specifying the URL enables the prebid caching feature.