Fix AmpAdApiHandler type check #5722
Conversation
| * @param {!Element} element | ||
| * @param {function()=} opt_noContentCallback | ||
| */ | ||
| constructor(baseInstance, element, opt_noContentCallback) { | ||
| /** @private {!AMP.BaseElement} */ | ||
|
|
||
| /** @private {!./amp-ad-3p-impl.AmpAd3PImpl|../../amp-a4a/0.1/amp-a4a.AmpA4A}*/ |
There was a problem hiding this comment.
The duplicated type hint here shouldn't be necessary.
There was a problem hiding this comment.
I think we need this. Even if CC managed to infer the type from the param, without a @private it won't consider it an instance variable.
There was a problem hiding this comment.
yes, i think this.baseInstance_ still need type check, like we still put a @private {!Element} beforethis.element_`.
There was a problem hiding this comment.
@private's not a type hint, we need it. But the {...} stuff should be inferred, right?
There was a problem hiding this comment.
I see. it did work with only @private. Hmmm the type check thing confused me so much 😢
| @@ -32,12 +32,13 @@ const TIMEOUT_VALUE = 10000; | |||
| export class AmpAdApiHandler { | |||
|
|
|||
| /** | |||
| * @param {!AMP.BaseElement} baseInstance | |||
| * @param {!./amp-ad-3p-impl.AmpAd3PImpl|../../amp-a4a/0.1/amp-a4a.AmpA4A} baseInstance | |||
There was a problem hiding this comment.
should this be {!...|!...} ? (note the second ! for the second union type)
There was a problem hiding this comment.
(same for the instance variable one)
| * @param {!Element} element | ||
| * @param {function()=} opt_noContentCallback | ||
| */ | ||
| constructor(baseInstance, element, opt_noContentCallback) { | ||
| /** @private {!AMP.BaseElement} */ | ||
|
|
||
| /** @private {!./amp-ad-3p-impl.AmpAd3PImpl|../../amp-a4a/0.1/amp-a4a.AmpA4A}*/ |
There was a problem hiding this comment.
I think we need this. Even if CC managed to infer the type from the param, without a @private it won't consider it an instance variable.
| @@ -161,9 +161,10 @@ export class AmpA4A extends AMP.BaseElement { | |||
| /** @private {?string} */ | |||
| this.experimentalNonAmpCreativeRenderMethod_ = null; | |||
|
|
|||
| this.lifecycleReporter_ = getLifecycleReporter(this, 'a4a'); | |||
| /** @public {!../../../ads/google/a4a/performance.AmpAdLifecycleReporter|!../../../ads/google/a4a/performance.NullLifecycleReporter} */ | |||
| @@ -84,8 +84,10 @@ export class AmpAd3PImpl extends AMP.BaseElement { | |||
| /** @private {?Promise} */ | |||
| this.layoutPromise_ = null; | |||
|
|
|||
| this.lifecycleReporter_ = getLifecycleReporter(this, 'amp'); | |||
| this.lifecycleReporter_.sendPing('adSlotBuilt'); | |||
| /** @public {!../../../ads/google/a4a/performance.AmpAdLifecycleReporter|!../../../ads/google/a4a/performance.NullLifecycleReporter} */ | |||
AmpAdApiHandler is calling
AmpAd3PImplorAmpA4Amethod, howeverBaseElementdon't have these method type so there will be type check errors. This is to fix the issue by restricting theAmpAdApiHandlerbaseInstanceto be eitherAmpAd3pImplorAmpA4. This also helps for code sharing #5356 later, so that I can access theirUIHandlerwithout type check errors.PTAL