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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰 Wait for story impl to resolve #13829
Conversation
@@ -117,7 +120,7 @@ export class AmpStoryAutoAds extends AMP.BaseElement { | |||
Services.extensionsFor(this.win)./*OK*/installExtensionForDoc( | |||
ampdoc, MUSTACHE_TAG); | |||
|
|||
ampStoryElement.getImpl().then(impl => { | |||
this.storyImplPromise_ = ampStoryElement.getImpl().then(impl => { |
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.
you might just return this promise, it will block the lifecycle of the element.
but please test it out if i am right
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.
You are right. changed.
@@ -71,6 +71,9 @@ export class AmpStoryAutoAds extends AMP.BaseElement { | |||
constructor(element) { | |||
super(element); | |||
|
|||
/** @private {?Promise} */ | |||
this.storyImplPromise_ = null; |
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.
remove
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.
oops. done
* wait for story impl * block on buildCallback * remove unused var
amp-story-auto-ads
had a race condition where it was setting a pointer to theamp-story
impl synchronously, even though thegetImpl()
method is async.This wraps that bit of code in a promise.