Introduces an optional mechanism for advanced advertising techniques #1214
Conversation
| 'Origin should have been validated'); | ||
| run(type, win, data); | ||
| if (configCallback) { | ||
| configCallback(data, () => { |
There was a problem hiding this comment.
Why not pass the data object, instead of counting on mutation?
configCallback(data, (data) => run(type, win, data));There was a problem hiding this comment.
I was thinking about it, but it makes the signature of the callback harder to document.
Is your intuition to do it anyway?
There was a problem hiding this comment.
Yah, I tend not to grok "mutate this object" APIs. Either passing the data (or returning it) makes things much more clear, but we can't support async calls with the return value.
There was a problem hiding this comment.
agree with @jridgewell here from an API standpoint. the closer the locality the better. easier to refactor (pure functions etc.)
There was a problem hiding this comment.
Cool.
I hope you are fine with me not copying the data, though :)
So, it isn't really pure.
There was a problem hiding this comment.
Oh yah, I don't expect anyone to do an immutable object. It just makes it easier to understand what's being passed as the configured object now.
cramforce
force-pushed
the
3p-config-callback
branch
from
a81960f
to
caa8ab9
Compare
| expect(called).to.be.false; | ||
| called = true; | ||
| expect(myWin).to.equal(win); | ||
| expect(myData).to.equal(myData); |
There was a problem hiding this comment.
Fixed to what it was supposed to be.
cramforce
force-pushed
the
3p-config-callback
branch
2 times, most recently
from
7fa6d07
to
477f628
Compare
| // Don't actually call setTimeout here. This should only serve as an | ||
| // example that is OK to call the done callback asynchronously. | ||
| setTimeout(function() { | ||
| done(); |
There was a problem hiding this comment.
Need to update the example.
|
LGTM. |
to manipulate incoming ad configuration in a callback function. This is only available for publishers who host the ad iframe themselves. With that they can obviously do whatever they want, but with this there is a larger incentive to stay within the AMP system and don't rely on implicit APIs (such as how we pass in configuration). Fixes ampproject#1210
cramforce
force-pushed
the
3p-config-callback
branch
from
477f628
to
d2a8e88
Compare
Introduces an optional mechanism for advanced advertising techniques
…to manipulate incoming ad configuration in a callback function.
This is only available for publishers who host the ad iframe themselves.
With that they can obviously do whatever they want, but with this there is a larger incentive to stay within the AMP system and don't rely on implicit APIs (such as how we pass in configuration).
Increases test coverage for integration.js.
Fixes #1210