Adagio Bid Adapter: external script compliance #11351
Conversation
osazos
force-pushed
the
adagio-feature/external-script
branch
from
9712247
to
7efe65c
Compare
- lock version - remove loading from localStorage (eval() fn)
osazos
force-pushed
the
adagio-feature/external-script
branch
from
7efe65c
to
29b6e6e
Compare
There was a problem hiding this comment.
This looks good besides this one point:
I think the "turning off adagio script load" won't work as expected because the time that the script is loaded is going to occur before the on page code to set bidderSettings will not be there yet.
initAdagio() occurs when the module is loaded.
Which will happen before pbjs.processQueue() is called.
Which if a pub wanted to turn it off, pbjs.bidderSettings.adagio.scriptVersion = 'none', it would likely occur inside of a pbjs.que function block.
There are probably ways around this
- putting
pbjs.bidderSettings.adagio.scriptVersion = 'none'outside of the queue block before prebid loads will probably work
But I wanted to mention this as I would imagine putting the bidderSettings logic inside of a queue block would be the usual way this is done.
Also, not related to this PR, but accessing the storage handler this early during module LOAD time has the same effect.
storage.getDataFromLocalStorage('adagio' ....
We have not given the publisher enough time to wait for pbjs.setConfig's, pbjs.bidderSettings, etc to occur.
This is a more widespread issue I think in many adapters, something to talk about perhaps!
Though, I may be totally wrong and this is handled as expected haha.
Are you saying you won't bid if your external script doesnt run? |
| - `storageAllowed`: Adagio uses browser local storage, explicit access to it must be configured _(since Prebid.js 7.x)_ | ||
| - `scriptVersion`: Adagio loads an additional script used for measurement. By default the adapter loads a specific version of this script, defined in the adapter itself. Two values can override this behavior: | ||
| - `latest`: allow to always get the more recent version of the script | ||
| - `none`: deactivates the feature, no script is loaded |
There was a problem hiding this comment.
please make this the default
| utilsMock.expects('logInfo').withExactArgs('Adagio: start script').never(); | ||
| utilsMock.expects('logWarn').withExactArgs('Adagio: no hash found.').once(); | ||
| utilsMock.expects('logWarn').withExactArgs('Adagio: invalid script found.').never(); | ||
| it('loads the latest external script, bypass the lock version', function() { |
There was a problem hiding this comment.
not sure i'm comfortable with this, we'll need to talk in committee
| } | ||
|
|
||
| const url = computeAdagioScriptUrl(adagiojsVersion); | ||
| loadExternalScript(url, BIDDER_CODE, undefined, undefined, { id: `adagiojs-${getUniqueIdentifierStr()}`, version: adagiojsVersion }); |
There was a problem hiding this comment.
we'll need to discuss if we're going to let a bidder do this, likely we will not, as that was the initial plan. Why not distribute your sidecar script independently of prebid or make it another module?
|
Thank you both for the prompt feedback. This pull request serves as an initial step in addressing this issue rather than attempting to impose a solution. @patmmccann, we want to avoid requesting our clients to add a script tag, as we perceive it as additional effort for both parties and a significant change in our way of doing. @robertrmartinez, I understand now. My tests yielded false positives, and I mistakenly believed that the storageManager functions waited for Prebid.js to be fully ready, this is not the case. The behavior is de-facto not compatible with the way we need to load the external script. |
|
Loading your script in RTD is absolutely reasonable and encouraged, RTD modules are exempted from the no script rule and publishers understand they may do such activity |
|
Great @patmmccann, we are currently working on a RTD module. Thx. |
Type of change
Description of change
This PR aims to align Adagio with the Prebid.js standards as requested in #10038 and #10653.
Note that we still have to load our script in a matter of simplicity for our clients. The changes that are done should fit the community requirements especially the fact that we don't use
Function()to load the cached version of script in localStorage.@patmmccann I would really appreciate your feedback before opening it for reviewing and write the public doc.