-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix lots of type errors and introduces assertElement. #4885
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>} */ | ||
|
@@ -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); | ||
|
@@ -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 | ||
*/ | ||
|
@@ -472,14 +472,15 @@ export class UrlReplacements { | |
: String(metric); | ||
}); | ||
} else { | ||
return Promise.resolve(String(metric)); | ||
return /** @type {!Promise<(string|undefined)>} */ ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reason. Unfortunately CC doesn't support complex type equivalencies on generics. |
||
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) { | ||
|
@@ -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 | ||
*/ | ||
|
@@ -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]; | ||
|
@@ -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); | ||
} | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,7 +116,7 @@ export class Viewer { | |
/** @private {boolean} */ | ||
this.overtakeHistory_ = false; | ||
|
||
/** @private {string} */ | ||
/** @private {!VisibilityState} */ | ||
this.visibilityState_ = VisibilityState.VISIBLE; | ||
|
||
/** @private {string} */ | ||
|
@@ -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 | ||
|
@@ -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) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merged my changes, this should be the only line that has a conflict There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh theres another conflict at the bottom i completely missed on line 1102 |
||
this.overtakeHistory_); | ||
dev().fine(TAG_, '- history:', this.overtakeHistory_); | ||
|
||
this.setVisibilityState_(this.params_['visibilityState']); | ||
|
@@ -237,9 +246,6 @@ export class Viewer { | |
// Wait for document to become visible. | ||
this.docState_.onVisibilityChanged(this.recheckVisibilityState_.bind(this)); | ||
|
||
/** @private {function()|undefined} */ | ||
this.messagingReadyResolver_ = undefined; | ||
|
||
/** | ||
* This promise will resolve when communications channel has been | ||
* established or timeout in 20 seconds. The timeout is needed to avoid | ||
|
@@ -287,9 +293,6 @@ export class Viewer { | |
this.isTrustedViewerOrigin_(this.win.location.ancestorOrigins[0])); | ||
trustedViewerPromise = Promise.resolve(trustedViewerResolved); | ||
} else { | ||
|
||
/** @private {!function(boolean)|undefined} */ | ||
this.trustedViewerResolver_ = undefined; | ||
// Wait for comms channel to confirm the origin. | ||
trustedViewerResolved = undefined; | ||
trustedViewerPromise = new Promise(resolve => { | ||
|
@@ -300,9 +303,6 @@ export class Viewer { | |
/** @const @private {!Promise<boolean>} */ | ||
this.isTrustedViewer_ = trustedViewerPromise; | ||
|
||
/** @private {!function(string)|undefined} */ | ||
this.viewerOriginResolver_ = undefined; | ||
|
||
/** @const @private {!Promise<string>} */ | ||
this.viewerOrigin_ = new Promise(resolve => { | ||
if (!this.isEmbedded()) { | ||
|
@@ -775,7 +775,8 @@ export class Viewer { | |
* @return {!Promise} | ||
*/ | ||
requestFullOverlay() { | ||
return this.sendMessageUnreliable_('requestFullOverlay', {}, true); | ||
return /** @type {!Promise} */ ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this need to be typecast? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because CC is bad at templates and this message can return null. Unfortunately the assert cannot produce the right type here. |
||
this.sendMessageUnreliable_('requestFullOverlay', {}, true)); | ||
} | ||
|
||
/** | ||
|
@@ -784,7 +785,8 @@ export class Viewer { | |
* @return {!Promise} | ||
*/ | ||
cancelFullOverlay() { | ||
return this.sendMessageUnreliable_('cancelFullOverlay', {}, true); | ||
return /** @type {!Promise} */ ( | ||
this.sendMessageUnreliable_('cancelFullOverlay', {}, true)); | ||
} | ||
|
||
/** | ||
|
@@ -793,8 +795,8 @@ export class Viewer { | |
* @return {!Promise} | ||
*/ | ||
postPushHistory(stackIndex) { | ||
return this.sendMessageUnreliable_( | ||
'pushHistory', {stackIndex}, true); | ||
return /** @type {!Promise} */ (this.sendMessageUnreliable_( | ||
'pushHistory', {stackIndex}, true)); | ||
} | ||
|
||
/** | ||
|
@@ -803,8 +805,8 @@ export class Viewer { | |
* @return {!Promise} | ||
*/ | ||
postPopHistory(stackIndex) { | ||
return this.sendMessageUnreliable_( | ||
'popHistory', {stackIndex}, true); | ||
return /** @type {!Promise} */ (this.sendMessageUnreliable_( | ||
'popHistory', {stackIndex}, true)); | ||
} | ||
|
||
/** | ||
|
@@ -833,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; | ||
}); | ||
}); | ||
|
@@ -921,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); | ||
|
@@ -1014,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) { | ||
|
@@ -1090,7 +1093,7 @@ function parseParams_(str, allParams) { | |
|
||
/** | ||
* Creates an error for the case where a channel cannot be established. | ||
* @param {!Error|string=} opt_reason | ||
* @param {*=} opt_reason | ||
* @return {!Error} | ||
*/ | ||
function getChannelError(opt_reason) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we need to change this too to "AssertFunctionByTypeName" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah nvm, looks like that doesn't work. i was hoping
AssertFunctionByTypeName
solves the inference issueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is just an utility to be able to use custom types like
Element