🐛 Properly indicate when FIE is done for no-signing #31530
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dvoytenko
suggested changes
Dec 10, 2020
src/friendly-iframe-embed.js
Outdated
| /** @private @const {!Promise} */ | ||
| this.winLoadedPromise_ = Promise.all([loadedPromise, this.whenReady()]); | ||
| if (this.ampdoc) { | ||
| this.whenReady().then(() => this.ampdoc.setReady()); | ||
| // TODO(ccordry): wait for renderComplete after no signing launch. | ||
| const readyPromise = this.spec.skipHtmlMerge |
There was a problem hiding this comment.
It's probably best to just use one promise here: whenRenderComplete(), but resolve renderComplete_ promise at the same time as readyPromise when this.spec.skipHtmlMerge. That way this new promise will always be "correct" from API point of view. Otherwise, right now, it could end up unresolved in some cases.
src/friendly-iframe-embed.js
Outdated
| // TODO(ccordry): wait for renderComplete after no signing launch. | ||
| const readyPromise = this.spec.skipHtmlMerge | ||
| ? this.whenRenderComplete() | ||
| : this.whenReady(); |
There was a problem hiding this comment.
Let's rename this function to avoid confusion. This should be whenRenderStarted().
powerivq
approved these changes
Dec 10, 2020
dvoytenko
suggested changes
Dec 11, 2020
| @@ -465,6 +489,13 @@ export class FriendlyIframeEmbed { | |||
| } else { | |||
| this.signals_.signal(CommonSignals.RENDER_START); | |||
| } | |||
|
|
|||
| // TODO(ccordry): remove when no-signing launched. | |||
There was a problem hiding this comment.
FYI: this class and this mode are used by most of our unit tests. So we'd need to switch them to streaming as well.
dvoytenko
approved these changes
Dec 11, 2020
Enriqe
pushed a commit
to Enriqe/amphtml
that referenced
this pull request
Dec 15, 2020
* signal end of transfer * tests * use one promise * fix tests * renaming * fix animation
This was referenced Jul 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tell FIE ampdoc that it is complete only after all elements have been transferred. Tested manually and if I am in no-signing + fie-resources this will solve the malformed json errors. This will probably cost us a bit of speed, but will iterate on fie-resource building logic in follow up.
Partial #27189