Skip to content

Commit

Permalink
Fix lots of type errors and introduces assertElement.
Browse files Browse the repository at this point in the history
- Gets rid of many specific `Element` types. In practice these aren't helpful, but introduce a lot of casting.
- Might have fixed on actualy bug in url-replacements.js

142 error(s), 6 warning(s), 93.81603250831178% typed
  • Loading branch information
cramforce committed Sep 9, 2016
1 parent 60d1c7b commit ca5c9e7
Show file tree
Hide file tree
Showing 17 changed files with 111 additions and 64 deletions.
5 changes: 5 additions & 0 deletions build-system/amp.extern.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,8 @@ twttr.widgets.createTweet;

var FB;
FB.init;

// Validator
var amp;
amp.validator;
amp.validator.validateUrlAndLog = function(string, doc, filter) {}
Binary file modified build-system/runner/dist/runner.jar
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.ampproject;

import com.google.common.collect.ImmutableList;
import com.google.javascript.jscomp.ClosureCodingConvention.AssertFunctionByTypeName;
import com.google.javascript.jscomp.CodingConvention;
import com.google.javascript.jscomp.CodingConvention.AssertionFunctionSpec;
import com.google.javascript.jscomp.CodingConventions;
Expand Down Expand Up @@ -45,9 +46,8 @@ public AmpCodingConvention(CodingConvention convention) {
return ImmutableList.of(
new AssertionFunctionSpec("user.assert", JSType.TRUTHY),
new AssertionFunctionSpec("dev.assert", JSType.TRUTHY),
new AssertionFunctionSpec("module$src$log.user.assert", JSType.TRUTHY),
new AssertionFunctionSpec("module$src$log.dev.assert", JSType.TRUTHY),
new AssertionFunctionSpec("Log$$module$src$log.prototype.assert", JSType.TRUTHY)
new AssertionFunctionSpec("Log$$module$src$log.prototype.assert", JSType.TRUTHY),
new AssertFunctionByTypeName("Log$$module$src$log.prototype.assertElement", "Element")
);
}

Expand Down
2 changes: 1 addition & 1 deletion src/event-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export function isLoaded(element) {
* Returns a promise that will resolve or fail based on the element's 'load'
* and 'error' events. Optionally this method takes a timeout, which will reject
* the promise if the resource has not loaded by then.
* @param {!Element} element
* @param {!EventTarget} element
* @param {number=} opt_timeout
* @return {!Promise<!Element>}
*/
Expand Down
2 changes: 1 addition & 1 deletion src/json.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export function getValueForExpr(obj, expr) {
* Parses the given `json` string without throwing an exception if not valid.
* Returns `undefined` if parsing fails.
* Returns the `Object` corresponding to the JSON string when parsing succeeds.
* @param {string} json JSON string to parse
* @param {*} json JSON string to parse
* @param {function(!Error)=} opt_onFailed Optional function that will be called with
* the error if parsing fails.
* @return {?JSONValueDef|undefined}
Expand Down
19 changes: 19 additions & 0 deletions src/log.js
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,25 @@ export class Log {
}
return shouldBeTrueish;
}

/**
* Throws an error if the first argument isn't an Element
*
* Otherwise see `assert` for usage
*
* @param {*} shouldBeElement
* @param {string=} opt_message The assertion message
* @param {...*} var_args Arguments substituted into %s in the message.
* @return {!Element} The value of shouldBeTrueish.
* @template T
*/
/*eslint "google-camelcase/google-camelcase": 2*/
assertElement(shouldBeElement, opt_message, var_args) {
const shouldBeTrueish = shouldBeElement && shouldBeElement.nodeType == 1;
return /** @type {!Element} */ (
this.assert(shouldBeTrueish, opt_message || 'Element expected',
var_args));
}

/**
* Asserts and returns the enum value. If the enum doesn't contain such a value,
Expand All @@ -261,6 +279,7 @@ export class Log {
* @return T
* @template T
*/
/*eslint "google-camelcase/google-camelcase": 2*/
assertEnumValue(enumObj, s, opt_enumName) {
for (const k in enumObj) {
if (enumObj[k] == s) {
Expand Down
6 changes: 3 additions & 3 deletions src/observable.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@ export class Observable {

/**
* Fires an event. All observers are called.
* @param {TYPE} event
* @param {TYPE=} opt_event
*/
fire(event) {
fire(opt_event) {
const handlers = this.handlers_;
for (let i = 0; i < handlers.length; i++) {
const handler = handlers[i];
handler(event);
handler(opt_event);
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/performance.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ import {getExistingServiceForWindow} from './service';

/**
* @param {!Window} window
* @return {!Performance}
* @return {!./service/performance-impl.Performance}
*/
export function performanceFor(window) {
return getExistingServiceForWindow(window, 'performance');
return /** @type {!./service/performance-impl.Performance}*/ (
getExistingServiceForWindow(window, 'performance'));
};
19 changes: 10 additions & 9 deletions src/service/url-replacements-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export class UrlReplacements {
/** @private {!RegExp|undefined} */
this.replacementExpr_ = undefined;

/** @private @const {!Object<string, function(*):*>} */
/** @private @const {!Object<string, function(*, *):*>} */
this.replacements_ = this.win_.Object.create(null);

/** @private @const {function():!Promise<?AccessService>} */
Expand Down Expand Up @@ -426,7 +426,7 @@ export class UrlReplacements {
* @template T
*/
getAccessValue_(getter, expr) {
return this.getAccessService_(this.win_).then(accessService => {
return this.getAccessService_().then(accessService => {
if (!accessService) {
// Access service is not installed.
user().error(TAG, 'Access service is not installed to access: ', expr);
Expand All @@ -441,8 +441,8 @@ export class UrlReplacements {
* The data for the timing events is retrieved from performance.timing API.
* If start and end events are both given, the result is the difference between the two.
* If only start event is given, the result is the timing value at start event.
* @param {string} startEvent
* @param {string=} endEvent
* @param {*} startEvent
* @param {*=} endEvent
* @return {!Promise<string|undefined>}
* @private
*/
Expand Down Expand Up @@ -472,14 +472,15 @@ export class UrlReplacements {
: String(metric);
});
} else {
return Promise.resolve(String(metric));
return /** @type {!Promise<(string|undefined)>} */ (
Promise.resolve(String(metric)));
}
}

/**
* Returns navigation information from the current browsing context.
* @param {string} attribute
* @return {!Promise<string|undefined>}
* @return {!Promise<undefined>|string}
* @private
*/
getNavigationData_(attribute) {
Expand All @@ -497,7 +498,7 @@ export class UrlReplacements {
* Sets the value resolver for the variable with the specified name. The
* value resolver may optionally take an extra parameter.
* @param {string} varName
* @param {function(*):*} resolver
* @param {function(*, *):*} resolver
* @return {!UrlReplacements}
* @private
*/
Expand Down Expand Up @@ -604,7 +605,7 @@ export class UrlReplacements {
/**
* Method exists to assist stubbing in tests.
* @param {string} name
* @return {function(*):*}
* @return {function(*, *):*}
*/
getReplacement_(name) {
return this.replacements_[name];
Expand All @@ -620,7 +621,7 @@ export class UrlReplacements {
if (additionalKeys && additionalKeys.length > 0) {
const allKeys = Object.keys(this.replacements_);
additionalKeys.forEach(key => {
if (allKeys[key] === undefined) {
if (this.replacements_[key] === undefined) {
allKeys.push(key);
}
});
Expand Down
40 changes: 26 additions & 14 deletions src/service/viewer-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export class Viewer {
/** @private {boolean} */
this.overtakeHistory_ = false;

/** @private {string} */
/** @private {!VisibilityState} */
this.visibilityState_ = VisibilityState.VISIBLE;

/** @private {string} */
Expand All @@ -125,7 +125,7 @@ export class Viewer {
/** @private {number} */
this.prerenderSize_ = 1;

/** @private {string} */
/** @private {!ViewportType} */
this.viewportType_ = ViewportType.NATURAL;

/** @private {number} */
Expand Down Expand Up @@ -164,6 +164,15 @@ export class Viewer {
/** @private {?time} */
this.firstVisibleTime_ = null;

/** @private {?Function} */
this.messagingReadyResolver_ = null;

/** @private {?Function} */
this.viewerOriginResolver_ = null;

/** @private {?Function} */
this.trustedViewerResolver_ = null;

/**
* This promise might be resolved right away if the current
* document is already visible. See end of this constructor where we call
Expand All @@ -188,8 +197,8 @@ export class Viewer {
this.isRuntimeOn_ = !parseInt(this.params_['off'], 10);
dev().fine(TAG_, '- runtimeOn:', this.isRuntimeOn_);

this.overtakeHistory_ = parseInt(this.params_['history'], 10) ||
this.overtakeHistory_;
this.overtakeHistory_ = !!(parseInt(this.params_['history'], 10) ||
this.overtakeHistory_);
dev().fine(TAG_, '- history:', this.overtakeHistory_);

this.setVisibilityState_(this.params_['visibilityState']);
Expand Down Expand Up @@ -766,7 +775,8 @@ export class Viewer {
* @return {!Promise}
*/
requestFullOverlay() {
return this.sendMessageUnreliable_('requestFullOverlay', {}, true);
return /** @type {!Promise} */ (
this.sendMessageUnreliable_('requestFullOverlay', {}, true));
}

/**
Expand All @@ -775,7 +785,8 @@ export class Viewer {
* @return {!Promise}
*/
cancelFullOverlay() {
return this.sendMessageUnreliable_('cancelFullOverlay', {}, true);
return /** @type {!Promise} */ (
this.sendMessageUnreliable_('cancelFullOverlay', {}, true));
}

/**
Expand All @@ -784,8 +795,8 @@ export class Viewer {
* @return {!Promise}
*/
postPushHistory(stackIndex) {
return this.sendMessageUnreliable_(
'pushHistory', {stackIndex}, true);
return /** @type {!Promise} */ (this.sendMessageUnreliable_(
'pushHistory', {stackIndex}, true));
}

/**
Expand All @@ -794,8 +805,8 @@ export class Viewer {
* @return {!Promise}
*/
postPopHistory(stackIndex) {
return this.sendMessageUnreliable_(
'popHistory', {stackIndex}, true);
return /** @type {!Promise} */ (this.sendMessageUnreliable_(
'popHistory', {stackIndex}, true));
}

/**
Expand Down Expand Up @@ -824,7 +835,7 @@ export class Viewer {
// it should resolve in milli seconds.
return timerFor(this.win).timeoutPromise(10000, cidPromise, 'base cid')
.catch(error => {
dev().error(error);
dev().error(TAG_, error);
return undefined;
});
});
Expand Down Expand Up @@ -912,7 +923,8 @@ export class Viewer {
return Promise.resolve();
}
if (eventType == 'broadcast') {
this.broadcastObservable_.fire(data);
this.broadcastObservable_.fire(
/** @type {!JSONType|undefined} */ (data));
return Promise.resolve();
}
dev().fine(TAG_, 'unknown message:', eventType);
Expand Down Expand Up @@ -1005,7 +1017,7 @@ export class Viewer {
* @param {string} eventType
* @param {*} data
* @param {boolean} awaitResponse
* @return {!Promise<*>|undefined}
* @return {?Promise<*>|undefined}
* @private
*/
sendMessageUnreliable_(eventType, data, awaitResponse) {
Expand Down Expand Up @@ -1081,7 +1093,7 @@ function parseParams_(str, allParams) {

/**
* Creates an error for the case where a channel cannot be established.
* @param {!Error=} opt_reason
* @param {*=} opt_reason
* @return {!Error}
*/
function getChannelError(opt_reason) {
Expand Down
19 changes: 10 additions & 9 deletions src/service/viewport-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export class Viewport {
/** @private @const {!Observable} */
this.scrollObservable_ = new Observable();

/** @private {?HTMLMetaElement|undefined} */
/** @private {?Element|undefined} */
this.viewportMeta_ = undefined;

/** @private {string|undefined} */
Expand Down Expand Up @@ -491,7 +491,7 @@ export class Viewport {
}

/**
* @return {?HTMLMetaElement}
* @return {?Element}
* @private
*/
getViewportMeta_() {
Expand Down Expand Up @@ -627,9 +627,7 @@ export class ViewportBindingDef {
* independent fixed layer.
* @return {boolean}
*/
requiresFixedLayerTransfer() {
return false;
}
requiresFixedLayerTransfer() {}

/**
* Register a callback for scroll events.
Expand Down Expand Up @@ -908,6 +906,9 @@ export class ViewportBindingNaturalIosEmbed_ {
/** @private {?Element} */
this.scrollMoveEl_ = null;

/** @private {?Element} */
this.endPosEl_ = null;

/** @private {!{x: number, y: number}} */
this.pos_ = {x: 0, y: 0};

Expand Down Expand Up @@ -956,7 +957,7 @@ export class ViewportBindingNaturalIosEmbed_ {
overflowY: 'auto',
webkitOverflowScrolling: 'touch',
});
setStyles(documentBody, {
setStyles(dev().assertElement(documentBody), {
overflowX: 'hidden',
overflowY: 'auto',
webkitOverflowScrolling: 'touch',
Expand Down Expand Up @@ -1185,7 +1186,7 @@ export class ViewportBindingNaturalIosEmbed_ {
* width=device-width,initial-scale=1,minimum-scale=1
* ```
* @param {string} content
* @return {!Object<string, string>}
* @return {!Object<string, (string|undefined)>}
* @private Visible for testing only.
*/
export function parseViewportMeta(content) {
Expand Down Expand Up @@ -1288,7 +1289,7 @@ function createViewport_(window) {
* @return {!Viewport}
*/
export function installViewportService(window) {
return getService(window, 'viewport', () => {
return /** @type !Viewport} */ (getService(window, 'viewport', () => {
return createViewport_(window);
});
}));
};
Loading

0 comments on commit ca5c9e7

Please sign in to comment.