🚀 [amp-story-player] Wait for first story to load before loading next ones #29703
Conversation
|
Hey @gmajoulet, @newmuis! These files were changed: |
|
I filed this ticket for the followup to make it easier to track: #29706 |
Re. event name: Considering that having an amp-story specific message is faster than using |
| updateCurrentIframe_(story) { | ||
| const iframeIdx = story[IFRAME_IDX]; | ||
| const iframeEl = this.iframes_[iframeIdx]; | ||
| this.layoutIframe_(story, iframeEl, VisibilityState.VISIBLE).then(() => { |
There was a problem hiding this comment.
This layoutIframe_ is a preventive operation in case the user swiped to navigate before this new current story started its layoutIframe_ (because it was waiting for the first story to load).
|
Friendly ping |
Enriqe
force-pushed
the
improve-iframe-loading
branch
from
ce15cc5 to
e0d8dc4
Compare
| * @private | ||
| */ | ||
| waitForStoryToLoadPromise_(iframeIdx) { | ||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
Did you decide to not implement a timeout because of the fallback you have where if the user navigates to the next story it still loads properly?
There was a problem hiding this comment.
So do you mean like returning Promise.race([promise, timeout]) where timeout is a Promise resolved by the next story when navigated to it?
There was a problem hiding this comment.
Yes, but I don't know if it's a good idea since it'd mean you're on a device that's struggling with the first story load, and you'd start loading the second one.
There was a problem hiding this comment.
Yeah, I thought about it but decided not to do it because when navigating it would cause slower loading times for the new story.
E.g.
- We have the initial load of the 1st, 2nd, and 3rd stories.
- User navigates, 1st story is still not finished loading
- Player resolves
timeout; making 2nd and 3rd story to start loading.
This would make the 2nd story load slower, because it would be loading at the same time as the 3rd story.
…should have been laid out already
Enriqe
force-pushed
the
improve-iframe-loading
branch
from
af760b2 to
05b6372
Compare
|
PTAL |
|
Friendly ping |
|
@gmajoulet friendly ping |
… ones (ampproject#29703) * wait for first story to load before loading next ones * address edge case where user swipes before subsequent stories are laid out * add private * typo * change event name * remove preventive layout for previous iframe, since by definition it should have been laid out already * add unit test for edge case * cleanups * fix types * renaming, clean up by calling cacheUrl once * reject previous promises * weird merge but ok * condense into one deferred
Fixes #29320
Loading 2 iframes initially involved a lot of changes (because of how other methods like
show()expect there to be 3 iframes) so I'm sending those in a follow up. (Tracked #29706)As seen on the first table, having a story specific event that delays the subsequent stories from being loaded makes the initial load time of the current story shorter.
Time for first story page to be loaded
storyLoadeddocumentLoadedThere is a downside though, loading subsequent stories take longer to finish pre-rendering.
Time for secondary stories to be pre-rendered
Quick calculations using different network connection emulations and with the same player sample page containing local stories.