BaseAdapter for the Prebid 0.x -> 1.x transition #1494

dbemiller · 2017-08-17T17:54:43Z

Type of change

Refactoring (no functional changes, no api changes)

Description of change

#1421 changes the APIs for both Adapters and Publishers. We're planning on cutting a 1.0 branch soon... and this PR aims to reduce the maintenance cost of that branch during the transition period.

In a nutshell: the singleRequestBidder is a new "base" bidder intended to support any adapters which make only one request to the server. Its Adapter-facing API is compatible with both Prebid 0.x and Prebid 1.x.

Any compatible Adapters can code against this Base in master. If they do, they won't have to duplicate changes across branches whenever they do Adapter code maintenance.

Still TODO:

User sync (waiting on Rubicon project improvement/user sync #1549 to merge)

At this point, any API feedback on the singleRequestBidder would be super useful. Everything else is just details.

…exusAst adapter onto it

…e multi-request architecture adapters.

…gle-request-bidder

snapwich · 2017-08-25T19:54:35Z

I like this. I'll set up some time to review it more in-depth, but it might not be until next week.

snapwich

Overall looking pretty good, but I left some feedback.

snapwich · 2017-08-31T16:18:55Z

src/ajax.js

@@ -5,6 +5,9 @@ var utils = require('./utils');
 const XHR_DONE = 4;
 let _timeout = 3000;

+export const GET = 'GET';
+export const POST = 'POST';


I don't think we need these. I think constants are useful when you're dealing with magic numbers or non-obvious strings, but the strings 'POST' and 'GET' are sufficiently descriptive and this makes an extra dependency for all the adapters.

Ironically... I actually added these because I couldn't remember where this ajax function was expecting GET or get. Figured this would save me from having to look it up the next time.

If "10" is "I refuse to use strings", and "1" is "I refuse to use constants", I'm like... a 6. Enough to challenge you here, but not very hard. How strong is your preference?

Also open to other peoples' feedback, if people want to vote.

If this were our API, then I'd probably be more open to the idea of adding these constants. But this is just a layer on top of an existing API that's already pretty standardized around using 'GET', 'POST', etc strings: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/open

aha... ok, that makes plenty of sense then. My javascript inexperience is showing itself... didn't know those were so well established.

I'll take the constants out. I should probably rename type to method, too... will take a pass for consistency, now that I know where the conventions are coming from.

snapwich · 2017-08-31T16:21:06Z

modules/appnexusAstBidAdapter.js

-          bidmanager.addBidResponse(placementCode, createBid(STATUS.NO_BID));
-        });
-      return;
+// TODO: Patch UserSync into the spec, once the UserSync PR has been merged.


Do you have any ideas how you would like UserSync to be implemented? Would it be another interface function of the bidderFactory or something that you would tag along into the interpretResponse function?

that's a good question... and I don't know. I actually chatted with our team about it, but didn't walk away with any super clear answers.

I'm not familiar enough with AdTech to know how it's done... but I could imagine three places where user syncs might come from.

Constant values. "Always ping this usersync endpoint on every auction"

Server-side logic: "Always ping this endpoint to get the usersync info. It will return one or more syncs"

Winning bids. "Because this bid made it into the auction, drop this usersync pixel"

Depending on which cases are worth supporting, I'd choose different APIs. It'll require adding one or more optional properties to the spec:

For (1), it'd be getSyncs()
For (2), it'd be getSyncs(done)
For (3), it'd be getSyncs(serverResponse)

In all cases, the return value (or callback argument) would just be an object with the data which we need to call registerSync in #1549.

Do you have any better background or opinion on it? I'm interested to hear what other adapter maintainers think too, if they see this.

snapwich · 2017-08-31T16:24:53Z

src/adapters/bidderFactory.js

+ * @param {BidderSpec} spec An object containing the bare-bones functions we need to make a Bidder.
+ */
+export default function newBidder(spec) {
+  return Object.assign(new Adapter(spec.code), {


We need to figure out how aliasing is going to work with this new pattern. Currently I have it calling new on the .constructor link of an existing instance. In this case it would be an instance of just plain Adapter, which means it wouldn't work.

Added support for these. I'm not sure this is the best way, though... so let me know if you have better ideas.

If we think this API is general enough to be the only base class, I might re-organize the code a bit so that "bidder creation" is in a separate file from "bidder registration".

I still haven't heard anyone weigh in on whether or not this handles all the 1.0 use-cases, though, or whether i'm overly-constraining adapters with it.

snapwich · 2017-08-31T16:26:59Z

src/adapters/bidderFactory.js

+ * Typical usage looks something like:
+ *
+ * const adapter = newBidder({
+ *   code: 'myBidderCode',


It would be nice and easy for implementers if we allowed aliases to be defined here somehow

e.g.

const adapter = newBidder({ code: 'rubicon', aliases: ['rubiconLite', 'roooobicon']

snapwich · 2017-08-31T16:31:24Z

modules/appnexusAstBidAdapter.js

 }

-adaptermanager.registerBidAdapter(new AppnexusAstAdapter(), 'appnexusAst', {


It might be nice to get rid of the adaptermanager dependency and have the bidderFactory be responsible for registering the new adapter.

I think part of our goal for this new interface to bidder adapter creation should be getting rid of any many of the imported dependencies as possible, that way the interface to creating and registering an adapter can be a singular call into newBidder rather than importing various modules and using them all separately. I think that's where adapter creators get confused, by our current disparate set of interfaces to different modules when creating an adapter.

This never even occurred to me... it's a great idea though

snapwich · 2017-08-31T16:33:47Z

src/adapters/bidderFactory.js

+}
+
+/**
+ * @typedef {object} ServerRequest


I think these typedefs are awesome and will help with a lot of the confusion that currently exists around what type of objects Prebid.js expects to operate with. Do you you think it'd be worthwhile making constructor functions that are used to create these objects?

e.g.

var serverRequest = new ServerRequest('GET', ENDPOINT, data);

I think this would help with data validation. We probably want to validate that the adapters are giving us back the correct data types. But rather than do stuff like if(!serverRequest.type || !serverRequest.endpoint ... we could just do if(!serverRequest instanceof ServerRequest) and the constructor function would could check for required parameters.

I've been thinking about this... but I'm not so convinced.

First: instanceof is a pretty flimsy check in javascript. For example:

var serverRequest = new ServerRequest(...); delete serverRequest.type serverRequest.type // Is undefined serverRequest instanceof Request // Is true

Second: it sounds like all the complex validation code has just moved into the constructor. Rather than if (!serverRequest.type), the constructor has to do if (!type). I can see some advantage in having the validation code centralized... but if that's the goal, I'd rather define it an isRequest(obj) function somewhere.

Third: constructors are a bad pattern. To be honest, I don't expect the drawbacks to affect this area much... but I do want to discourage them from spreading around the codebase. There's some good discussion on them in (2) and (3) here

Data validation is an open question as far as I'm concerned, though.

I left it out here because I was thinking of how React totally disabled all runtime property validation in the prod version of their library because it was found to be a notable performance drain. Granted, React apps have hundreds (or even thousands) of nested components... so it may not be a fair role model. Since everyone in adtech seems to be super concerned about performance, though, I figured I'd leave it out.

Worth mentioning: Assuming management schedules time for me to work on it, I have a followup PR I hope to make for "simple, unified adapter tests." Basically I want to make it so that adapters can define some JSON files for valid bid params and expected server responses, and we can centralize all the actual adapter test code. One of those tests would be making sure that the spec only returns request objects with valid params. If we do it at test time, then it doesn't need to be done at runtime.

It's true, there are ways to subvert instanceof checks. The reason I like them is because, as you said, type validation is expensive and instanceof checks are not. Using a constructor function to define a type allows you to incur a one-time hit for the validation of an object (and then use instanceof thereafter), as opposed to isRequest(obj) which is a many-time hit for each time that object is used. That's the problem React encountered as they had to validate propTypes each type a component instance was used.

Either way, this isn't a make or break issue for me, it's just something I think of every time I see if (obj.property .... Also, if we did do this we'd have to expose the constructor functions on the public API as well so external adapters could pass the instanceof checks, which isn't terrible but it's more of a hassle than just using typedef.

yeah... I do understand where you're coming from, but I think I'd prefer to hold off on this for now.

I'd feel better talking about how to optimize runtime validation after people see the tests I have in mind, if they still think they have value.

snapwich · 2017-08-31T17:04:50Z

src/adapters/bidderFactory.js

+export default function newBidder(spec) {
+  return Object.assign(new Adapter(spec.code), {
+    callBids: function(bidsRequest) {
+      if (!bidsRequest.bids || !bidsRequest.bids.filter) {


This looks like an array check. Would be more clear as if (!Array.isArray(bidsRequest.bids) ) {

snapwich · 2017-08-31T17:19:38Z

src/adapters/bidderFactory.js

+          });
+      }
+
+      const bidRequests = bidsRequest.bids.filter(filterAndWarn);


These names are confusing, especially here. I think bidRequests is fine, but bidsRequest I think is called bidderRequest in other parts of Prebid.

snapwich · 2017-08-31T17:44:55Z

src/adapters/bidderFactory.js

+          bids = spec.interpretResponse(response);
+        } catch (err) {
+          logError(`Bidder ${spec.code} failed to interpret the server's response. Continuing without bids`, null, err);
+          onResponse();


I think the calling of onResponse here is a good use case for a finally clause so you don't have to scatter as many calls around.

I'd wrap all this code in a try block and put the onResponse in a finally so it's obvious that it will always be called.

I like the goal here... but I'm really trying not to catch unexpected errors either.

For example, while writing this I originally had a bug in my addBidUsingRequestMap function where something was unexpectedly undefined.

I easily may never have caught that if it'd been inside the try block.

snapwich · 2017-08-31T17:50:57Z

src/adapters/bidderFactory.js

+        const bidRequestMap = {};
+        bidRequests.forEach(bid => {
+          bidRequestMap[bid.bidId] = bid;
+        });


Why is this bidRequestMap recreated for each successful request? Couldn't it just be made once in the same context as the bidRequests?

…l be broken.

snapwich · 2017-09-01T16:40:56Z

src/adapters/bidderFactory.js

+/**
+ * @typedef {object} BidRequest
+ *
+ * @param {string} bidId A string which uniquely identifies this BidRequest in the current Auction.


These should be @property instead of @param for typedefs.

snapwich · 2017-09-01T16:46:04Z

src/mediaTypes.js

+ * @typedef {('native'|'video'|'banner')} MediaType
+ */
+
+/** @ype MediaType */


I think you have a typo here

…quest-related properties for consistency with the native object.

… in the merges somewhere.

mkendall07

Overall I like the strategy. What scares me is the amount of code change in the adapters. Overall LGTM though.

mkendall07 · 2017-09-06T18:54:45Z

src/adapters/bidderFactory.js

+ */
+
+/**
+ * @typedef {object} BidderSpec An object containing the adapter-specific functions needed to


maybe just me but I like to see these typedefs inline with the code.

I moved these, but I'm uh.... not sure I did what you had in mind.

The first mention of BidderSpec is inside an @param {BidderSpec} spec. That block documents the function... so the closest places for the typedef are either before or after the function implementation.

I moved it before the function implementation... which might be better? I dunno though.

Let me know if you have something better in mind. The BidderSpec is used at the top-level functions... and all the other typedef blocks in here are used inside the BidderSpec.

mkendall07 · 2017-09-06T19:01:38Z

modules/appnexusAstBidAdapter.js

+export const spec = {
+  code: BIDDER_CODE,
+  supportedMediaTypes: [VIDEO, NATIVE],
+  aliases: ['dummy'],


mkendall07 · 2017-09-06T19:09:45Z

modules/appnexusAstBidAdapter.js

+   * @return {Bid[]} An array of bids which were nested inside the server.
+   */
+  interpretResponse: function(serverResponse) {
+    const parsed = JSON.parse(serverResponse);


suppose the base adapter could do the parse, unless we need custom error handling in the adapter (I don't see any here).

…e top.

jaiminpanchal27 · 2017-09-07T14:11:47Z

src/adapters/bidderFactory.js

+      //
+      // In Prebid 1.0, this will be simplified to use the `addBidResponse` and `done` callbacks.
+      const placementCodesHandled = {};
+      function addBidWithCode(placementCode, bid) {


We are using two names for same thing in our codebase i.e placementCode and adUnitCode. From 1.0 we should just use code or adUnitCode
Gist for adUnit changes https://gist.github.com/mkendall07/51ee5f6b9f2df01a89162cf6de7fe5b6#adunit-obj

Did not know that... I removed placementCode from all the comments and local variable names.

I think I still have to use bidRequest.placementCode in Prebid 0.x, though, right? Or is there somewhere else I should be getting it from?

old review

…built * 'master' of https://github.com/prebid/Prebid.js: (46 commits) Serverbid alias (prebid#1560) Add user sync to process for approving adapter PRs (prebid#1457) fix travis build (prebid#1595) Rubicon project improvement/user sync (prebid#1549) Adding Orbitsoft adapter (prebid#1378) Fix renderer test for new validation rule (prebid#1592) Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577) Add support for video stream context (prebid#1483) Invalidate bid if matching bid request not found (prebid#1575) allow adapters to set default adserverTargeting for specific bid (prebid#1568) Custom granularity precision should honor 0 if it is passed in closes prebid#1479 (prebid#1591) BaseAdapter for the Prebid 0.x -> 1.x transition (prebid#1494) Add a version to the Criteo adapter (prebid#1573) Allow bundling from node.js or with new gulp task bundle-to-stdout (prebid#1570) Add url.parse option to not decode the whole URL (prebid#1480) Tremor Video Bid Adapter (prebid#1552) Yieldmo bid adapter (prebid#1415) Switch `gulp docs` to build its output using documentation.js (prebid#1545) Increment pre version Prebid 0.28.0 Release ...

* Added a base adapter for single-request adapters, and ported the appnexusAst adapter onto it * Renamed the SingleRequestBidder to BidderFactory. Updated it to handle multi-request architecture adapters. * Added a unit test for the delayExecution function. * Made newBidder a default import. Added some unit tests. * Added more tests. * Added more tests, and fixed a few bugs. * Changed an error to a log message. Fixed a small bug. * Did the no-brainer improvements from PR comments. * Added spec-level support for aliases and mediaTypes. Aliases may still be broken. * Added support for aliases. Added more tests * Cleaned up some unnecessary code. * Removed the GET/POST constants. Fixed some typos, and renamed some Request-related properties for consistency with the native object. * Re-added some code for outstream rendering, which was apparently lost in the merges somewhere. * Removed confusing use of this * Fixed lint error * Moved JSON parsing into the bidderFactory, and moved the JSDocs to the top. * Removed placementCode from everywhere I could.

Added a base adapter for single-request adapters, and ported the appn…

c1080dd

…exusAst adapter onto it

dbemiller added the in progress label Aug 17, 2017

Renamed the SingleRequestBidder to BidderFactory. Updated it to handl…

70fcf51

…e multi-request architecture adapters.

dbemiller changed the title ~~Added a base adapter for single-request adapters, and ported the appn…~~ Adding a BaseAdapter for the 0.x -> 1.x transition Aug 18, 2017

dbemiller changed the title ~~Adding a BaseAdapter for the 0.x -> 1.x transition~~ Adding a BaseAdapter for the Prebid 0.x -> 1.x transition Aug 18, 2017

dbemiller changed the title ~~Adding a BaseAdapter for the Prebid 0.x -> 1.x transition~~ BaseAdapter for the Prebid 0.x -> 1.x transition Aug 18, 2017

dbemiller added 5 commits August 18, 2017 12:49

Added a unit test for the delayExecution function.

211e7fc

Merged from master. Fixed conflicts.

6246eaf

Made newBidder a default import. Added some unit tests.

d509150

Added more tests.

8d4ebc1

Merge branch 'master' of https://github.com/prebid/Prebid.js into sin…

7ffc16f

…gle-request-bidder

dbemiller requested review from snapwich and mkendall07 August 21, 2017 15:32

dbemiller added 3 commits August 22, 2017 10:16

Added more tests, and fixed a few bugs.

1f23044

Merged from master. Fixed a conflict.

8d1efa6

Changed an error to a log message. Fixed a small bug.

a627a43

mkendall07 self-assigned this Aug 24, 2017

Merged from master, and fixed conflicts.

ff99ff6

snapwich previously requested changes Aug 31, 2017

View reviewed changes

dbemiller added 4 commits August 31, 2017 15:02

Did the no-brainer improvements from PR comments.

c246aa6

Added spec-level support for aliases and mediaTypes. Aliases may stil…

6c80c3d

…l be broken.

Added support for aliases. Added more tests

37399c7

Cleaned up some unnecessary code.

1b42995

snapwich reviewed Sep 1, 2017

View reviewed changes

Removed the GET/POST constants. Fixed some typos, and renamed some Re…

5c3d645

…quest-related properties for consistency with the native object.

This was referenced Sep 1, 2017

Yieldmo bid adapter #1415

Merged

Concurrent auctions for Prebid 1.0 #1421

Closed

Merged from master. Fixed conflicts.

31899f4

dbemiller added 3 commits September 6, 2017 12:18

Re-added some code for outstream rendering, which was apparently lost…

735f2bc

… in the merges somewhere.

Removed confusing use of this

dd4e595

Fixed lint error

25f822a

bretg added the Prebid 1.0 label Sep 6, 2017

mkendall07 reviewed Sep 6, 2017

View reviewed changes

Moved JSON parsing into the bidderFactory, and moved the JSDocs to th…

e63f2d6

…e top.

jaiminpanchal27 reviewed Sep 7, 2017

View reviewed changes

Removed placementCode from everywhere I could.

13d3911

snapwich mentioned this pull request Sep 12, 2017

single-request-bidder: updates to bidderFactory and converted rubiconBidAdapter #1580

Closed

1 task

mkendall07 approved these changes Sep 13, 2017

View reviewed changes

matthewlane approved these changes Sep 13, 2017

View reviewed changes

mkendall07 merged commit a4d3ce7 into master Sep 13, 2017

dbemiller mentioned this pull request Sep 15, 2017

Add JSDoc for pbjs.getAllWinningBids #1566

Merged

8 tasks

matthewlane deleted the single-request-bidder branch October 6, 2017 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BaseAdapter for the Prebid 0.x -> 1.x transition #1494

BaseAdapter for the Prebid 0.x -> 1.x transition #1494

dbemiller commented Aug 17, 2017 •

edited

Loading

snapwich commented Aug 25, 2017

snapwich left a comment

snapwich Aug 31, 2017

dbemiller Aug 31, 2017 •

edited

Loading

snapwich Sep 1, 2017

dbemiller Sep 1, 2017 •

edited

Loading

snapwich Aug 31, 2017

dbemiller Aug 31, 2017

snapwich Aug 31, 2017

dbemiller Sep 1, 2017

snapwich Aug 31, 2017

snapwich Aug 31, 2017 •

edited

Loading

dbemiller Aug 31, 2017

snapwich Aug 31, 2017

dbemiller Sep 1, 2017 •

edited

Loading

snapwich Sep 1, 2017

dbemiller Sep 5, 2017 •

edited

Loading

snapwich Aug 31, 2017

snapwich Aug 31, 2017

snapwich Aug 31, 2017 •

edited

Loading

dbemiller Aug 31, 2017

snapwich Aug 31, 2017

snapwich Sep 1, 2017

snapwich Sep 1, 2017

mkendall07 left a comment

mkendall07 Sep 6, 2017

dbemiller Sep 6, 2017

mkendall07 Sep 6, 2017

mkendall07 Sep 6, 2017

jaiminpanchal27 Sep 7, 2017

dbemiller Sep 7, 2017 •

edited

Loading

		}

		adaptermanager.registerBidAdapter(new AppnexusAstAdapter(), 'appnexusAst', {

BaseAdapter for the Prebid 0.x -> 1.x transition #1494

BaseAdapter for the Prebid 0.x -> 1.x transition #1494

Conversation

dbemiller commented Aug 17, 2017 • edited Loading

Type of change

Description of change

snapwich commented Aug 25, 2017

snapwich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapwich Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapwich Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkendall07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

dbemiller commented Aug 17, 2017 •

edited

Loading

dbemiller Aug 31, 2017 •

edited

Loading

dbemiller Sep 1, 2017 •

edited

Loading

snapwich Aug 31, 2017 •

edited

Loading

dbemiller Sep 1, 2017 •

edited

Loading

dbemiller Sep 5, 2017 •

edited

Loading

snapwich Aug 31, 2017 •

edited

Loading

dbemiller Sep 7, 2017 •

edited

Loading