Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

amp-bind: Wait for init before scanAndApply #21763

Merged

Conversation

dreamofabear
Copy link

Fixes race condition that can result in duplicate binding records e.g. in the case where amp-list renders before amp-bind's tree walk completes.

@@ -381,6 +381,19 @@ export class Bind {
* @return {!Promise}
*/
scanAndApply(addedElements, removedElements, timeout = 2000) {
return this.initializePromise_.then(() => {
return this.rescan_(addedElements, removedElements, timeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these have to run sequentially? Can we just apply the latest rescan?

Copy link
Author

@dreamofabear dreamofabear Apr 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, thought about that e.g. skip elements with attribute i-amphtml-binding during tree walk.

However, initialization also looks for and sets up amp-bind-macro which is needed if the rescanned contents references it. So we'd need to extract that first.

Added a TODO so we can revisit later.

@dreamofabear dreamofabear merged commit 84a002d into ampproject:master Apr 9, 2019
@dreamofabear dreamofabear deleted the bind-scanAndApply-init-race branch April 9, 2019 14:26
dreamofabear pushed a commit that referenced this pull request Apr 10, 2019
* Wait for bind.init before scanAndApply.

* Add todo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants