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
Fix race and speed up amp-list/amp-bind interaction #22938
Fix race and speed up amp-list/amp-bind interaction #22938
Conversation
/to @jridgewell |
Services.bindForDocOrNull(this.element).then(bind => { | ||
if (bind) { | ||
return bind.scanAndApply([this.element], [this.element]); | ||
return bind.rescan([this.element], [this.element], {'apply': true}); |
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.
@cvializ I think this was broken for awhile. 🤷♂
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.
Time to write an integration test 😬
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.
This looks fine, but the interactions are too complicated for me to really follow.
// Scan `addedElements` and their children for elements with bindings. | ||
const elementsToScan = addedElements.slice(); | ||
const elementsToScan = addedElements.filter(el => | ||
el.hasAttribute('i-amphtml-binding') |
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.
Why are you filtering here?
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.
Minor optimization. We only want to scan elements with [i-amphtml-binding]
attribute, and elements in addedElements
may not have it.
scanElement_()
iterates over an element's attributes, so this should be a small perf win.
* Refactor scanAndApply and rename to rescan. * Fix types. * Remove old skip. * Use string keys to be safe, add early returns. * rescan() should wait for macros in fast mode. * Fix a few bugs. * Fix tests, clean up. * Lint. * Fix types. * More readability changes. * Skip rescan in amp-list if elements have no binding attributes. * Add amp-list tests (WIP). * Only skip scan of children, update example. * Better comments. * Add tests for new Bind.rescan options. * Improve perf of binding limit detection and early-out.
Fixes #22341, partial for #21901.
amp-list[binding=refresh]
and amp-bind's initial DOM scan.Bind.rescan
in amp-list if new children have no binding marker attributes ([i-amphtml-binding]
).Bind.rescan
to only wait for a small part of Bind's initialization.Bind.rescan
to fix/support amp-date-picker's usage.