✨Create new extension - AMP-smartlinks#20494
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
c3d3d94 to
1504d07
Compare
|
I signed it! |
|
Yes!
…On Fri, Jan 25, 2019 at 3:00 PM Phil Winchester ***@***.***> wrote:
I signed it!
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#20494 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AP2wKQ0-WW2I1WUGOolB3cCPhE2ZDtgvks5vG2J2gaJpZM4aPzZp>
.
|
extensions/amp-smartlinks/0.1/test/validator-amp-smartlinks.out
Outdated
Show resolved
Hide resolved
extensions/amp-smartlinks/0.1/test/validator-amp-smartlinks.html
Outdated
Show resolved
Hide resolved
extensions/amp-smartlinks/0.1/test/validator-amp-smartlinks.html
Outdated
Show resolved
Hide resolved
|
Thanks for the review @honeybadgerdontcare. I addressed your validator comment, please let me know if you spot anything else! |
|
@PhilWinchester Thank you for the PR. I am working on reviewing it, but it's a pretty big one, please allow me some time to review the change. |
zhouyx
left a comment
There was a problem hiding this comment.
@PhilWinchester Left some comments. Is it possible to reuse the LinkRewrite class that already exists? I found most class can be shared with the amp-skimlink service, please let me know if there's any concern. Thank you.
extensions/amp-smartlinks/0.1/link-rewriter/link-replacement-cache.js
Outdated
Show resolved
Hide resolved
extensions/amp-smartlinks/0.1/link-rewriter/link-rewriter-manager.js
Outdated
Show resolved
Hide resolved
| * @private | ||
| */ | ||
| initLinkRewriter_() { | ||
| const options = {linkSelector: this.linkmateOptions_.linkSelector}; |
There was a problem hiding this comment.
@erwinmombay could you help me understand if closure compiler is smart enough to understand that the linkSelector here equals to the linkSelector here
There was a problem hiding this comment.
@PhilWinchester So I asked @erwinmombay offline. We don't think that closure compilier is smart enough to understand that the options is the same to the LinkerRewriter options params without a typedef. This won't be an issue right now because all fieldName is reserved today, but it will be an issue when closure compiler start to obfuscated the fieldName.
We can either switch to using a JsonObject. But given only linkSelector is supported, can we instead pass a string to the constructor? If you think that makes sense I'm fine with a following PR to address the comment.
There was a problem hiding this comment.
@zhouyx
Do you mean doing something like this?
const options = /** @type {JsonObject} */
({'linkSelector': this.linkmateOptions_.linkSelector});I also assume that this part of the extension will need to be changed to if/when the affiliate link changes are added to the AMP project and the link-rewriter service is deprecated. I'm not sure if that would happen sooner than the closure compiler changes. Thanks for your help!
There was a problem hiding this comment.
nvm the link-rewriter code isn't expecting a JsonObject. I think I will follow up later with these changes.
There was a problem hiding this comment.
Even with the affiliate link changes, we are going to heavily rely on the existing link-rewriter service. As said I'd imaging this class to be kept and reused.
There was a problem hiding this comment.
i'll review again once @zhouyx comments about reusing the code from skimlinks extension is address. I know that the individual who wrote that extension initially had the idea and implementation of including that logic into runtime for reuse so maybe it might be worth discussing with him. My worries about importing code from that extension is that now you have this dependency on that extension which might break or change the way your extension behaves if they modify it.
|
CLAs look good, thanks! |
@alabiaga That's a really good point. Can we reach out to the previous contributor to move the code to |
ba13888 to
e888ec5
Compare
|
Thanks for the feedback and tips everyone! Any update on what is going to happen with the |
honeybadgerdontcare
left a comment
There was a problem hiding this comment.
validation changes look good.
8515240 to
02a44b0
Compare
I reached out to @slocka and awaiting his response. Thanks. |
|
The integration test failure in travis seems to be unrelated to my changes (couldn't replicate locally). What's the best way to rerun tests? |
I restarted it for you and will monitor it. |
|
I'm running into issues with the one of the comments above (#20494 (comment)), when actually triggering the event the key isn't registered in the /**
* API call to indicate a page load event happened
* @private
*/
postPageImpression_() {
// When using layout='nodisplay' manually trigger CustomEventReporterBuilder
this.signals().signal(CommonSignals.LOAD_START);
const builder = new CustomEventReporterBuilder(this.element);
const payload = this.buildPageImpressionPayload_();
builder.track('page-impression', ENDPOINTS.PAGE_IMPRESSION_ENDPOINT);
builder.setTransportConfig(dict({
'beacon': true,
'xhrpost': false,
'image': true,
// 'useBody': true,
}));
// builder.setExtraUrlParams(payload);
const reporter = builder.build();
reporter.trigger('page-impression', dict({
'data': JSON.stringify(payload),
}));
}Separately, any update on the |
|
@llanka or @zhouyx are best suited to answer your question about that API
I wouldn't rely on that change to be submitted before submitting this. I would keep in contact with @slocka to see his progression on that rewrite. I would submit this with the dependency on the skimlinks link rewriter logic and follow up with @slocka to make sure that this dependency is removed eventually. With the proper tests in place, you can still be notified on whether a change on their end will require changes to your extensions but that is something you don't want to deal with. |
|
Hello, so I was discussing with @lannka , and we noticed that we are getting a lot more interesting in affiliate linking within AMP. And in fact, we had this proposal in the past, but got de-prioritized: #9276 Could you look through that, and see if it would work for your use case? We were thinking it may be better to make a more general solution on our end, that platforms can integrate with 😄 Thank you! cc/ @alabiaga |
Could you please point me to the |
@alabiaga I am happy to move the LinkRewriter related code common to amp-skimlinks and amp-smartlinks to the src folder like it initially was. I would like confirmation that the migration is still worth doing considering @torch2424 last message? Should I wait for more visibility on #9276 before making the migration? |
…impression request (#12)
792a071 to
cac7783
Compare
zhouyx
left a comment
There was a problem hiding this comment.
@PhilWinchester Thank you! One last comment from me. Feel free to address in separate PR.
|
@alabiaga Minified compilation was broken by this PR. See https://travis-ci.org/ampproject/amphtml/jobs/493915905#L610 Sadly, it's too expensive to closure-compile all extensions during PR jobs, so we only detect these breaks on |
|
Sorry for breaking things :( |
|
Not a problem Phil. I am looking into this and will update you and we can merge it again. |
|
@rsimha @alabiaga Do you think it is due to amp-smartlinks trying to import modules from another extension. And there may be a race condition between when these two extensions gets built. @PhilWinchester No worries, we should document better to always test the compiled version locally. |
|
@PhilWinchester I spoke with @zhouyx and she has a fix in mind that we are playing around with. Will have an update for you soon. Apologies for the delay. |
|
@PhilWinchester pls add the following line in https://github.com/ampproject/amphtml/blob/master/build-system/tasks/compile.js#L199 I verified that it prod version of binary will build fine with that change but pls verify yourself as well. Thanks Thanks to @zhouyx and @erwinmombay for their help on this fix. |
Resolves ampproject#19470 ## What it does Create an extension that lets Narrativ offer our publishers our Linkmate service in AMP pages. The extension scans the page for any outbound links the publisher wants targeted and sends them to our API for monetization. Upon API response we use the `link-rewriter` service to map that response to the links in the initial payload. When a user clicks one of the monetized links we swap the href with the monetized link. More about Linkmate [here](http://docs.narrativ.com/en/stable/linkmate.html#linkmate).
Resolves ampproject#19470 ## What it does Create an extension that lets Narrativ offer our publishers our Linkmate service in AMP pages. The extension scans the page for any outbound links the publisher wants targeted and sends them to our API for monetization. Upon API response we use the `link-rewriter` service to map that response to the links in the initial payload. When a user clicks one of the monetized links we swap the href with the monetized link. More about Linkmate [here](http://docs.narrativ.com/en/stable/linkmate.html#linkmate).
Resolves #19470
What it does
Create an extension that lets Narrativ offer our publishers our Linkmate service in AMP pages. The extension scans the page for any outbound links the publisher wants targeted and sends them to our API for monetization. Upon API response we use the
link-rewriterservice to map that response to the links in the initial payload. When a user clicks one of the monetized links we swap the href with the monetized link. More about Linkmate here.