-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noop?
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.
Fixed to what it was supposed to be.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update the example.
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.
Done.
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
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