✨[amp-story-player] Install viewer integration script when inside the player #26719
Conversation
| @@ -1016,6 +1016,7 @@ export class AmpStory extends AMP.BaseElement { | |||
| .then(() => { | |||
| this.markStoryAsLoaded_(); | |||
| this.initializeLiveStory_(); | |||
| this.initializeStoryPlayer_(); | |||
There was a problem hiding this comment.
What's the timeout before the amp-story-player and its messaging API throws and error because the AMP document didn't respond to the handshake?
What happens on the amp-story-player side if the amp-story.js script takes a few seconds to load, and then the this.whenPagesLoaded_(PAGE_LOAD_TIMEOUT_MS)) times out?
There was a problem hiding this comment.
What's the timeout before the amp-story-player and its messaging API throws and error because the AMP document didn't respond to the handshake?
What happens on the amp-story-player side if the amp-story.js script takes a few seconds to load, and then the this.whenPagesLoaded_(PAGE_LOAD_TIMEOUT_MS)) times out?
While the script is loading we would show the loader/poster image of the first story, and the story will be displayed when the script is loaded. If this.whenPagesLoaded_(PAGE_LOAD_TIMEOUT_MS)) times out, it would display an error in the console (which we haven't set yet), but it doesn't block laying out the story.
There was a problem hiding this comment.
Sorry, my question was about the amp-story-player. It was triggering some errors if the amp-viewer-integration script was not loaded, wouldn't it trigger the same errors if the amp-viewer-integration is loaded too late?
What's the timeout on the player side of the viewer, before it throws one of these errors?
There was a problem hiding this comment.
Oh I see what you mean... Yeah it will throw if no messaging channel has been initialized in 20s...
Were you thinking of adding additional an additional timeout at the player level?
There was a problem hiding this comment.
If the story doesn't load in 20s we have a problem, that sgtm :)) Thanks for investigating
|
This pull request fixes 1 alert when merging 4f26d11 into da1fb54 - view on LGTM.com fixed alerts:
|
| @@ -1016,6 +1016,7 @@ export class AmpStory extends AMP.BaseElement { | |||
| .then(() => { | |||
| this.markStoryAsLoaded_(); | |||
| this.initializeLiveStory_(); | |||
| this.initializeStoryPlayer_(); | |||
There was a problem hiding this comment.
If the story doesn't load in 20s we have a problem, that sgtm :)) Thanks for investigating
This reverts commit 3e9e741.
Enriqe
force-pushed
the
player-fragment-param
branch
from
a6194e4
to
519206c
Compare
|
This pull request fixes 1 alert when merging 519206c into deed39a - view on LGTM.com fixed alerts:
|
Tracker #26308
Task #26694