-
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
create adapter adyoulike #1155
create adapter adyoulike #1155
Conversation
src/adapters/adyoulike.js
Outdated
const _VERSION = "0.1"; | ||
|
||
const baseAdapter = Adapter.createNew('adyoulike'); | ||
const bidRequests = {}; |
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.
Keeping state out here will be buggy. What would happen if callBids
gets called twice before your ajax request returns? For example, suppose a page makes more than one cal to requestBids
.
You can make it safe if you use newResponseHandler(requests)
as your ajax callback (where newResponseHandler(requests)
returns the handleResponse
function, with the requests stored in the closure scope)
You'll also want to submit a documentation PR for the bidder params page. |
I'm seeing CORS errors when testing:
My test page is below:
|
Could you use this placement ID for your test: |
@jbAdyoulike please fix the linting errors. |
@dbemiller done |
@dbemiller Could you give me the request body and response body of this endpoint This error looks strange, Following what I have from my side : |
@dbemiller Ok, It was a misconfiguration on my placement, it should works now. |
src/adapters/adyoulike.js
Outdated
try { | ||
return window.top.document.referrer; | ||
} catch (e) { } | ||
return document.referrer; |
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.
Generally, it's considered bad practice to use try/catch for control flow: https://softwareengineering.stackexchange.com/a/189225
This can be written linearly by testing for undefined (the same goes for getCanonicalUrl
).
src/adapters/adyoulike.js
Outdated
|
||
/* Get current page canonical url */ | ||
function getCanonicalUrl() { | ||
if (utils.inIframes) { |
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.
This is undefined... did you mean to call utils.inIframe()
here?
Needs just a few changes as far as I can tell... but I do want to complement you especially for writing such pretty code :). Super clear and easy to read. |
@dbemiller It's a pleasure ;) I have created the PR for the bidder params page prebid/prebid.github.io#252 |
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.
LGTM
* create adapter adyoulike * move bidRequests in callBids scope * update adyoulike.js and adyoulike_spec.js syntax * refactor getReferrerUrl and getCanonicalUrl
Hi @protonate what do we have to do to have our name updated on prebid.org? |
@ayl-julien @protonate is away for a little while, but submit a PR against this repo to change it. |
* create adapter adyoulike * move bidRequests in callBids scope * update adyoulike.js and adyoulike_spec.js syntax * refactor getReferrerUrl and getCanonicalUrl
Type of change
Description of change
[email protected]
Other information