🚀 amp-bind: Use querySelectorAll to quickly find all bound elements#32851
🚀 amp-bind: Use querySelectorAll to quickly find all bound elements#32851jridgewell merged 16 commits intomasterfrom
Conversation
| ? toArray(root.querySelectorAll('[i-amphtml-binding]')) | ||
| : []; | ||
|
|
||
| // Confusingly, the old TreeWalker hit the root node. We need to match that behavior. |
There was a problem hiding this comment.
Did it have any effect? I.e. is it harmful if we start skipping?
There was a problem hiding this comment.
No tests fail, but I don't know if we just never tested this case.
|
This solution is very smart! Replacing the TreeWalker with a BindWalker, presenting a similar interface so it fits right in. Have you thought about creating an entirely separate (and much simpler) flow for Galaxy brain idea: can you simply call |
jridgewell
left a comment
There was a problem hiding this comment.
Have you thought about creating an entirely separate (and much simpler) flow for addBindingsForNodes_ that skips all the chunking (but retains quota). Most of the logic in that flow seems necessary when using querySelectorAll.
There are already two paths with slowScan and fastScan, adding a new path made my head hurt. Retrofitting the slow path to actually be very fast was the easiest approach.
Galaxy brain idea: can you simply call fastScan_?
I couldn't get it to work, and didn't want to dedicate the whole week to this PR.
| ? toArray(root.querySelectorAll('[i-amphtml-binding]')) | ||
| : []; | ||
|
|
||
| // Confusingly, the old TreeWalker hit the root node. We need to match that behavior. |
There was a problem hiding this comment.
No tests fail, but I don't know if we just never tested this case.
* Add amp-bind optimizer See ampproject/amphtml#32851 * Apply suggestions from code review Co-authored-by: Sebastian Benz <sebastian.benz@gmail.com> * Add tests Co-authored-by: Sebastian Benz <sebastian.benz@gmail.com>
|
LMK what you think about an approach more similar to this: #33057 |
Co-Authored-By: Jake Fried <samouri@users.noreply.github.com>
|
Done. |
This reverts commit e4f9bdd.
|
@samouri: Ok, after attempting for 2 more days to get the tests to pass, I reverted the "simple" approach and kept the |
samouri
left a comment
There was a problem hiding this comment.
What was difficult about making fastScan_ work with tests? Was it related to quota logic or skipping amp-list children logic?
Please add a TODO before merging to switch to fastScan_ in the future
| /* opt_tagName */ 'input' | ||
| { | ||
| tag: 'input', | ||
| insertQuerySelectorAttr: useQuerySelector, |
There was a problem hiding this comment.
From purely a practical standpoint, do you think it would be cleaner to make a module level optimized var consumed by the createElement helper to conditionally add the css class? It breaks DI, but cuts down on boilerplate in each test.
This detects if amp-bind is SSRd to insert
i-amphtml-bindingattributes on any elements which have bindings. If so, we'll do a simplequerySelectorAllinstead of aTreeWalkerwalk.Part of #27590