✨Create new extension - AMP-smartlinks #20967

PhilWinchester · 2019-02-20T21:04:13Z

Resolves #19470

History

This PR was initially merged here (#20494) and then reverted here (#20891) because it broke master due to it's reliance on another extension. The changes in this PR are the same as 20494, with the addition to compile.js described here: #20494 (comment)

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.

Gregable

FYI: My review only addresses validation of the tag. It does not cover the javascript code or decisions about how the extension should work.

extensions/amp-smartlinks/validator-amp-smartlinks.protoascii

extensions/amp-smartlinks/amp-smartlinks.md

PhilWinchester · 2019-02-20T21:56:54Z

Hi @zhouyx @alabiaga,

I re-opened my changes here if you don't mind taking another look 😃. Added your comment here.

PhilWinchester · 2019-02-21T22:29:38Z

@zhouyx @alabiaga

Sorry to pester you guys all over again, just hoping to close out this PR as quickly as reasonable!

Thanks, Phil

zhouyx · 2019-02-21T22:41:09Z

Thank you @PhilWinchester Have tested locally with compiled file

gulp dist --extensions=amp-smartlinks
gulp serve --compiled

PhilWinchester · 2019-02-21T22:55:34Z

@zhouyx Yep! I tested with those commands and gulp dist --fortesting. No errors came up.

c-nichols · 2019-02-22T18:39:04Z

@Gregable @zhouyx given this was previously merged, what would you like addressed before re-merging vs. in a followup?

Gregable · 2019-02-22T22:36:11Z

@c-nichols I didn't notice that this was previously merged. Apologies for the moving goalposts.

As @honeybadgerdontcare mentioned, it might be difficult to make these changes later if there is observed usage by publishers, as we wouldn't want to make a breaking change.

I'd lean towards making the one change I recommended as my guess is that the modification is small. However, If you feel comfortable with the risk that you will be stuck with the current syntax, I'm OK with that. Consider my comment advisory. I can't speak for @zhouyx

My comments can be considered advisory.

c-nichols · 2019-02-25T14:29:54Z

Thanks @Gregable - we will make that change and then hopefully final approvals will come in quickly ;)

PhilWinchester · 2019-02-25T14:58:23Z

Hi @alabiaga & @zhouyx ,

I've updated the validator per conversation above and addressed the issue from the previous PR. Is it possible to get a quick review on this and close these changes out?

extensions/amp-smartlinks/0.1/amp-smartlinks.js

extensions/amp-smartlinks/0.1/linkmate-options.js

PhilWinchester · 2019-02-25T18:16:09Z

Thanks for the quick feedback @zhouyx @alabiaga. I've addressed your comments if you don't mind taking another look!

extensions/amp-smartlinks/0.1/test/test-amp-smartlinks.js

PhilWinchester · 2019-02-25T20:26:55Z

@alabiaga @zhouyx addressed your comments. If you could take another look 😄

extensions/amp-smartlinks/0.1/linkmate-options.js

extensions/amp-smartlinks/amp-smartlinks.md

PhilWinchester · 2019-02-25T21:38:10Z

@alabiaga @zhouyx Updated!

PhilWinchester · 2019-02-26T19:27:41Z

Hi @alabiaga @zhouyx,

Is this bundle size failure something I should worry about? I haven't noticed this test failing until this latest push.

alabiaga · 2019-02-26T21:18:09Z

@PhilWinchester let's pull from master and do another push for this PR to retrigger that failure.

* adding example page and amp-smartlinks to bundles.config * creating amp-smartlinks scaffolding * BAM-2584 adding hardcoded POC implementation of amp-smartlinks * adding new constants file and linkmate file/class * adding linkmate call and association working with link-rewrite service * add amp config call and update workflow to use those values * setup initial amp-smartlinks and linkmate workflow * adding new config variables for link attribute and selector * add tests for amp-smartlinks * add test for linkmate.js * add more thorough anchorList check in runSmartlinks * adding code comments, new constants structure and better options validation * update amp-smartlinks to have helpful information * updating global vars and cleaning up main files * clean up tests and add more explicit type assertions * clean up jsdoc tags

…ith userAssert (#4) * BAM-2585 whitelist navigation import and fix type errors * replace user.assert with userAssert

…impression request (#12)

…#14)

…RITER_ID

)

…mes in linkmate-options

c-nichols · 2019-02-28T14:13:11Z

@alabiaga tests passed 🎆 is this good to merge?

* cl/235984006 Revision bump for #21124 * cl/236141405 Validating ssr class values and attributes for transformed AMP * cl/236207670 Validating SSR layout should use the SSR CalculateLayout * cl/236240396 Revision bump for #20967 * cl/236242005 Revision bump for #20992 * cl/236357124 Allow `meta name=amp-recaptcha-input` * cl/236374806 Allow `meta name=amp-list-load-more` * cl/236486003 Limit depth of recursion in CSS parsing. * cl/236922121 n/a * cl/236944297 allow i-amphtml-sizer for transformed amp-story * eslint fixes

* creating new amp-smartlinks extension * BAM-2584 AMP-Smartlinks (#3) * adding example page and amp-smartlinks to bundles.config * creating amp-smartlinks scaffolding * BAM-2584 adding hardcoded POC implementation of amp-smartlinks * adding new constants file and linkmate file/class * adding linkmate call and association working with link-rewrite service * add amp config call and update workflow to use those values * setup initial amp-smartlinks and linkmate workflow * adding new config variables for link attribute and selector * add tests for amp-smartlinks * add test for linkmate.js * add more thorough anchorList check in runSmartlinks * adding code comments, new constants structure and better options validation * update amp-smartlinks to have helpful information * updating global vars and cleaning up main files * clean up tests and add more explicit type assertions * clean up jsdoc tags * BAM-2585 Whitelist import, fix type errors, and replace user.assert with userAssert (ampproject#4) * BAM-2585 whitelist navigation import and fix type errors * replace user.assert with userAssert * BAM-2585 fix validator and type check (ampproject#5) * more validator fixes (ampproject#6) * BAM-2585 Fix validator, copyright, and whitespace (ampproject#7) * alphabetize validator, fix whitespace, and add valid tag * update year in copyright statement * BAM-2585 move `link-rewriter` to import statements and updating types (ampproject#8) * BAM-2585 remove link-rewriter and switch to importing from skimlinks extension * clean up promise chain and more descriptive API comments * update xhr to pull from ampdoc.win and add types to constants.js * fix type in buildPageImpressionPayload_ * update validator with new empty value check (ampproject#9) * update validator for exclusive-links * switch page-impression API request to customEventReporter (ampproject#11) * BAM-2585 fix jsdoc in linkmate-options and unnecessary param in page_impression request (ampproject#12) * fix jsdoc for linkmate params and make runLinkmate more readable (ampproject#13) * add try/catch on amp_config fetch and updated constants.js * remove bad type in constants.js and add check for existing shop-links (ampproject#14) * add check for auction_id in mapLinks and add jsdoc for SMARTLINKS_REWRITER_ID * fix type notation in constants.js and linkmate-options.js * fix indentation in example file * add note to README describing link-rewriter priority queue behavior (ampproject#15) * add exception to compile.js for amp-skimlinks * update validator to use empty value as indicator linkmate param * fix validator and linkmate-options to use new config style (ampproject#16) * updating tests for linkmate-options.js * remove redundant userAssert in linkmate-options * update tests to reflect config changes * update tests to send accurate config params * update readme to refelct config changes and more accurate function names in linkmate-options

* cl/235984006 Revision bump for ampproject#21124 * cl/236141405 Validating ssr class values and attributes for transformed AMP * cl/236207670 Validating SSR layout should use the SSR CalculateLayout * cl/236240396 Revision bump for ampproject#20967 * cl/236242005 Revision bump for ampproject#20992 * cl/236357124 Allow `meta name=amp-recaptcha-input` * cl/236374806 Allow `meta name=amp-list-load-more` * cl/236486003 Limit depth of recursion in CSS parsing. * cl/236922121 n/a * cl/236944297 allow i-amphtml-sizer for transformed amp-story * eslint fixes

* creating new amp-smartlinks extension * BAM-2584 AMP-Smartlinks (ampproject#3) * adding example page and amp-smartlinks to bundles.config * creating amp-smartlinks scaffolding * BAM-2584 adding hardcoded POC implementation of amp-smartlinks * adding new constants file and linkmate file/class * adding linkmate call and association working with link-rewrite service * add amp config call and update workflow to use those values * setup initial amp-smartlinks and linkmate workflow * adding new config variables for link attribute and selector * add tests for amp-smartlinks * add test for linkmate.js * add more thorough anchorList check in runSmartlinks * adding code comments, new constants structure and better options validation * update amp-smartlinks to have helpful information * updating global vars and cleaning up main files * clean up tests and add more explicit type assertions * clean up jsdoc tags * BAM-2585 Whitelist import, fix type errors, and replace user.assert with userAssert (ampproject#4) * BAM-2585 whitelist navigation import and fix type errors * replace user.assert with userAssert * BAM-2585 fix validator and type check (ampproject#5) * more validator fixes (ampproject#6) * BAM-2585 Fix validator, copyright, and whitespace (ampproject#7) * alphabetize validator, fix whitespace, and add valid tag * update year in copyright statement * BAM-2585 move `link-rewriter` to import statements and updating types (ampproject#8) * BAM-2585 remove link-rewriter and switch to importing from skimlinks extension * clean up promise chain and more descriptive API comments * update xhr to pull from ampdoc.win and add types to constants.js * fix type in buildPageImpressionPayload_ * update validator with new empty value check (ampproject#9) * update validator for exclusive-links * switch page-impression API request to customEventReporter (ampproject#11) * BAM-2585 fix jsdoc in linkmate-options and unnecessary param in page_impression request (ampproject#12) * fix jsdoc for linkmate params and make runLinkmate more readable (ampproject#13) * add try/catch on amp_config fetch and updated constants.js * remove bad type in constants.js and add check for existing shop-links (ampproject#14) * add check for auction_id in mapLinks and add jsdoc for SMARTLINKS_REWRITER_ID * fix type notation in constants.js and linkmate-options.js * fix indentation in example file * add note to README describing link-rewriter priority queue behavior (ampproject#15) * add exception to compile.js for amp-skimlinks * update validator to use empty value as indicator linkmate param * fix validator and linkmate-options to use new config style (ampproject#16) * updating tests for linkmate-options.js * remove redundant userAssert in linkmate-options * update tests to reflect config changes * update tests to send accurate config params * update readme to refelct config changes and more accurate function names in linkmate-options

* cl/235984006 Revision bump for ampproject#21124 * cl/236141405 Validating ssr class values and attributes for transformed AMP * cl/236207670 Validating SSR layout should use the SSR CalculateLayout * cl/236240396 Revision bump for ampproject#20967 * cl/236242005 Revision bump for ampproject#20992 * cl/236357124 Allow `meta name=amp-recaptcha-input` * cl/236374806 Allow `meta name=amp-list-load-more` * cl/236486003 Limit depth of recursion in CSS parsing. * cl/236922121 n/a * cl/236944297 allow i-amphtml-sizer for transformed amp-story * eslint fixes

googlebot added the cla: yes label Feb 20, 2019

Gregable previously requested changes Feb 20, 2019

View reviewed changes

extensions/amp-smartlinks/validator-amp-smartlinks.protoascii Outdated Show resolved Hide resolved

extensions/amp-smartlinks/amp-smartlinks.md Outdated Show resolved Hide resolved

extensions/amp-smartlinks/amp-smartlinks.md Show resolved Hide resolved

alabiaga suggested changes Feb 25, 2019

View reviewed changes

extensions/amp-smartlinks/0.1/amp-smartlinks.js Outdated Show resolved Hide resolved

extensions/amp-smartlinks/0.1/linkmate-options.js Outdated Show resolved Hide resolved

zhouyx suggested changes Feb 25, 2019

View reviewed changes

extensions/amp-smartlinks/0.1/linkmate-options.js Outdated Show resolved Hide resolved

zhouyx reviewed Feb 25, 2019

View reviewed changes

extensions/amp-smartlinks/0.1/test/test-amp-smartlinks.js Outdated Show resolved Hide resolved

zhouyx reviewed Feb 25, 2019

View reviewed changes

Gregable approved these changes Feb 25, 2019

View reviewed changes

alabiaga reviewed Feb 25, 2019

View reviewed changes

extensions/amp-smartlinks/0.1/linkmate-options.js Outdated Show resolved Hide resolved

zhouyx reviewed Feb 25, 2019

View reviewed changes

extensions/amp-smartlinks/amp-smartlinks.md Outdated Show resolved Hide resolved

alabiaga approved these changes Feb 25, 2019

View reviewed changes

zhouyx approved these changes Feb 25, 2019

View reviewed changes

amp-bundle-size bot requested a review from dreamofabear February 25, 2019 22:53

PhilWinchester added 4 commits February 26, 2019 16:21

creating new amp-smartlinks extension

a9fbcb3

BAM-2585 Whitelist import, fix type errors, and replace user.assert w…

f43cb4b

…ith userAssert (#4) * BAM-2585 whitelist navigation import and fix type errors * replace user.assert with userAssert

BAM-2585 fix validator and type check (#5)

2b8aa7e

Phil Winchester and others added 19 commits February 26, 2019 16:21

update validator with new empty value check (#9)

ad10f15

update validator for exclusive-links

3b4e402

switch page-impression API request to customEventReporter (#11)

70252ab

BAM-2585 fix jsdoc in linkmate-options and unnecessary param in page_…

e3bf48a

…impression request (#12)

fix jsdoc for linkmate params and make runLinkmate more readable (#13)

1d6b56c

add try/catch on amp_config fetch and updated constants.js

ab53c04

remove bad type in constants.js and add check for existing shop-links (…

93cb495

…#14)

add check for auction_id in mapLinks and add jsdoc for SMARTLINKS_REW…

4bea8fe

…RITER_ID

fix type notation in constants.js and linkmate-options.js

21c54ab

fix indentation in example file

98eb2f1

add note to README describing link-rewriter priority queue behavior (#15

8bcaaa5

)

add exception to compile.js for amp-skimlinks

2ba6477

update validator to use empty value as indicator linkmate param

3c88906

fix validator and linkmate-options to use new config style (#16)

1ef376f

updating tests for linkmate-options.js

903dc90

remove redundant userAssert in linkmate-options

4e05183

update tests to reflect config changes

557e1cf

update tests to send accurate config params

e04bbb2

update readme to refelct config changes and more accurate function na…

d2e6bec

…mes in linkmate-options

PhilWinchester force-pushed the amp-smartlinks-0.1 branch from 8605851 to d2e6bec Compare February 26, 2019 21:22

alabiaga merged commit 6a78bcb into ampproject:master Feb 28, 2019

twifkak added a commit to twifkak/amphtml that referenced this pull request Mar 7, 2019

cl/236240396 Revision bump for ampproject#20967

a7a8dd5

twifkak mentioned this pull request Mar 7, 2019

Validator rollup #21305

Merged

PhilWinchester mentioned this pull request Mar 20, 2019

🐛amp-smartlinks fix bug in mapLinks_ function and attribute behavior #21503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Create new extension - AMP-smartlinks #20967

✨Create new extension - AMP-smartlinks #20967

PhilWinchester commented Feb 20, 2019 •

edited

Loading

Gregable left a comment

PhilWinchester commented Feb 20, 2019

PhilWinchester commented Feb 21, 2019

zhouyx commented Feb 21, 2019

PhilWinchester commented Feb 21, 2019

c-nichols commented Feb 22, 2019

Gregable commented Feb 22, 2019

c-nichols commented Feb 25, 2019

PhilWinchester commented Feb 25, 2019

PhilWinchester commented Feb 25, 2019

PhilWinchester commented Feb 25, 2019

PhilWinchester commented Feb 25, 2019

PhilWinchester commented Feb 26, 2019

alabiaga commented Feb 26, 2019

c-nichols commented Feb 28, 2019

✨Create new extension - AMP-smartlinks #20967

✨Create new extension - AMP-smartlinks #20967

Conversation

PhilWinchester commented Feb 20, 2019 • edited Loading

History

What it does

Gregable left a comment

Choose a reason for hiding this comment

PhilWinchester commented Feb 20, 2019

PhilWinchester commented Feb 21, 2019

zhouyx commented Feb 21, 2019

PhilWinchester commented Feb 21, 2019

c-nichols commented Feb 22, 2019

Gregable commented Feb 22, 2019

c-nichols commented Feb 25, 2019

PhilWinchester commented Feb 25, 2019

PhilWinchester commented Feb 25, 2019

PhilWinchester commented Feb 25, 2019

PhilWinchester commented Feb 25, 2019

PhilWinchester commented Feb 26, 2019

alabiaga commented Feb 26, 2019

c-nichols commented Feb 28, 2019

PhilWinchester commented Feb 20, 2019 •

edited

Loading