⛓Linker PT 2 #17616
⛓Linker PT 2 #17616
Conversation
| * before they are ready. | ||
| * @private | ||
| */ | ||
| initLinker_() { |
There was a problem hiding this comment.
let's separate this into a separate file linker-manager.js
| const vendorConfig = this.config_['linkers'][name]; | ||
|
|
||
| if (vendorConfig['enabled'] !== true) { | ||
| user().warn(TAG, `linker config for ${name} is not enabled and` + |
There was a problem hiding this comment.
this should be a debugging message
| } | ||
|
|
||
| const ids = vendorConfig['ids']; | ||
| dev().assert(ids, |
| * @param {./amp-analytics.AmpAnalytics} analytics | ||
| * @param {JsonObject} config | ||
| */ | ||
| constructor(analytics, config) { |
There was a problem hiding this comment.
let's avoid the reverse dependency on analytics
| * @param {!Element} element | ||
| * @private | ||
| */ | ||
| linkerCallback_(element) { |
| */ | ||
| init() { | ||
| if (!this.config_['linkers']) { | ||
| return; |
There was a problem hiding this comment.
can we have the optin check here to return early?
might be worth to just make a copy of enabled linkers in the config.
| } | ||
|
|
||
| // See if any domains match. | ||
| for (let i = 0; i < domains.length; i++) { |
There was a problem hiding this comment.
domains.indexOf(hostname) >= 0
There was a problem hiding this comment.
how about domains.includes()
src/service/navigation.js
Outdated
| @@ -42,6 +42,11 @@ const EVENT_TYPE_CONTEXT_MENU = 'contextmenu'; | |||
| /** @private @const {string} */ | |||
| const ORIG_HREF_ATTRIBUTE = 'data-a4a-orig-href'; | |||
|
|
|||
| /** @enum {number} */ | |||
| export const Priority = { | |||
| LINKER: 2, | |||
| import {Priority} from '../../../../src/service/navigation'; | ||
| import {Services} from '../../../../src/services'; | ||
|
|
||
| describe('Linkers', () => { |
| manager.init(); | ||
|
|
||
| return Promise.all(manager.allLinkerPromises_).then(() => { | ||
| const res1 = (manager.resolvedLinkers_['testLinker1']).split('~'); |
There was a problem hiding this comment.
avoid exposing resolvedLinkers_ to test, we can test the strings in a.href.
| * Called on click on any anchor element. Adds linker param if a match for | ||
| * given linker configuration. | ||
| * @param {!Element} element | ||
| * @private |
There was a problem hiding this comment.
I tried this, but if you add @visibleForTesting and it is used in the class it throws an error
There was a problem hiding this comment.
I think you will need to remove @private and also the ending _ in method name.
| const linkerNames = Object.keys(this.config_['linkers']); | ||
|
|
||
| if (!linkerNames.length) { | ||
| return user().error(TAG, |
There was a problem hiding this comment.
It might worth to support syntax below to completely disable linkers:
{
linkers: false
}
So, we might not want to print this error.
There was a problem hiding this comment.
Should we check both? linkers: false is good config, but linkers: {} is bad?
There was a problem hiding this comment.
Actually I think this is covered by line 65. if (!this.config_['linkers']) should cover the linkers: false case and exit before this code
There was a problem hiding this comment.
honestly i don't see strong reason to now allow linkers: {}, especially with increasing binary size.
| }); | ||
| }); | ||
|
|
||
| if (this.allLinkerPromises_.length) { |
* add priority * analytics changes * tests * example * fix import * types * move files * move files * lint * add enabled flag * add enabled to example * fix types * lint * comma * whitelist dep * add legacy opt-in * remove reverse dep * better warnings * lint * comments * includes * comments * make sure array * fix domains check * remove check * visible for test * bad rebase
Introduces the
Linkerlogic intoamp-analytics.