Url replacement clickxy#4153
Url replacement clickxy#4153keithwrightbos wants to merge 9 commits intoampproject:masterfrom google:url-replacement-clickxy
Conversation
…evaluated if used
validated via type check system. Additionally adds document click
listener which will expand href of anchor target.
Added replacement service to experiments.js which ensures it is fully
typed check. Fixed numerous errors in other files to ensure type check
executed on modified portions. Managed to get type check failures down
to the following within the file which are mostly unrelated. At some
point this quarter, AMP will be moving to full type checking. The
following changes were not included as they were only to facilitate type
checks.
Changes not staged for commit:
modified: src/animation.js
modified: src/curve.js
modified: src/experiments.js
modified: src/time.js
modified: src/transition.js
modified: src/url-replacements.js
modified: src/viewer.js
modified: src/viewport.js
modified: src/vsync.js
src/service/url-replacements-impl.js:68: ERROR - assignment to property getAccessService_ of UrlReplacements$$module$src$service$url_replacements_impl
found : function (Window): Promise<(AccessService|null)>
required: function ((Window|null)): (Promise<(AccessService|null)>|null)
this.getAccessService_ = accessServiceForOrNull;
^
src/service/url-replacements-impl.js:116: ERROR - inconsistent return type
found : Promise<string>
required: Promise<(string|undefined)>
return viewerFor(this.win_).getReferrerUrl();
^
src/service/url-replacements-impl.js:188: ERROR - Property get never defined on UserNotificationManager
return service.get(opt_userNotificationId);
^
src/service/url-replacements-impl.js:192: ERROR - Property get never defined on Cid
return cid.get({
^
src/service/url-replacements-impl.js:204: ERROR - restricted index type
found : *
required: string
user.assert(variants[experiment] !== undefined,
^
src/service/url-replacements-impl.js:207: ERROR - restricted index type
found : *
required: string
const variant = variants[experiment];
^
src/service/url-replacements-impl.js:331: ERROR - inconsistent return type
found : (Promise<(AccessService|null)>|null)
required: Promise<(string|undefined)>
return this.getAccessValue_(accessService => {
^
src/service/url-replacements-impl.js:332: ERROR - Property getAccessReaderId never defined on AccessService
return accessService.getAccessReaderId();
^
src/service/url-replacements-impl.js:340: ERROR - inconsistent return type
found : (Promise<(AccessService|null)>|null)
required: Promise<(string|undefined)>
return this.getAccessValue_(accessService => {
^
src/service/url-replacements-impl.js:341: ERROR - Property getAuthdataField never defined on AccessService
return accessService.getAuthdataField(field);
^
src/service/url-replacements-impl.js:347: ERROR - inconsistent return type
found : Promise<string>
required: Promise<(string|undefined)>
return viewerFor(this.win_).getViewerOrigin();
^
src/service/url-replacements-impl.js:353: ERROR - Property getTotalEngagedTime never defined on Activity
return activity.getTotalEngagedTime();
^
src/service/url-replacements-impl.js:415: ERROR - actual parameter 1 of module$src$event_helper.loadPromise does not match formal parameter
found : Window
required: Element
return loadPromise(this.win_).then(() => {
^
src/service/url-replacements-impl.js:491: ERROR - assignment
found : {async: (function (...*): Promise<(string|undefined)>|undefined), sync: (function (...*): (string|undefined)|null)}
required: {async: function (...*): Promise<(string|undefined)>, sync: function (...*): (string|undefined)}
this.replacements_[varName] =
^
src/service/url-replacements-impl.js:684: ERROR - restricted index type
found : string
required: number
if (allKeys[key] === undefined) {
^
src/service/url-replacements-impl.js:807: ERROR - inconsistent return type
found : *
required: UrlReplacements$$module$src$service$url_replacements_impl
return getService(window, 'url-replace', () => {
src/service/url-replacements-impl.js
Outdated
| return (typeof params[param] !== 'undefined') ? | ||
| params[param] : | ||
| defaultValue; | ||
| return (typeof val !== 'undefined') ? val : defaultValue; |
There was a problem hiding this comment.
Please invert this so there's not a double negative.
|
Back to @jridgewell for review. Thanks! |
| if (!this.win_.document) { | ||
| dev.warn('no win.document?! testing?'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Should this be in capture phase, so it works when propagation is stopped?
Should it be on document.documentElement?
|
Ping. |
|
Apologies, have been working on higher priority items. Will have changed On Jul 27, 2016 5:02 PM, "Justin Ridgewell" notifications@github.com
|
|
Not a problem, just going through my backlog. |
Implementation of document click listener expanding anchor target href including new X/Y click location. See I2I #3665
Enforces sync/async via type check though that is not currently enabled.
May want to merge within document click listener already created within src/document-click.js... thoughts?