#4942: re-test entire selector on mutation in initialize helper
#4943
Conversation
twschiller
changed the title
initialize helper
Codecov Report
@@ Coverage Diff @@
## main #4943 +/- ##
==========================================
+ Coverage 59.08% 59.11% +0.02%
==========================================
Files 963 963
Lines 29043 29043
Branches 5558 5558
==========================================
+ Hits 17161 17168 +7
+ Misses 11882 11875 -7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
src/vendors/initialize.js
Outdated
| // Check if the match applies now that the document has been updated. The handles cases where an ancestor was | ||
| // added/modified causing an element on the page to now match. This strictly isn't an "initialization", as the | ||
| // element wasn't just added. But conceptually, it corresponds to the selector now matching | ||
| matches.push(...$safeFind(msobserver.selector, options.target).toArray()); |
There was a problem hiding this comment.
If the original implementation misses some cases, then we can drop the entire block of code prior to this, right? That block tried to be efficient by checking exactly the parts the changed, but if we just check the whole tree at the end anyway, the optimization becomes duplicate.
There was a problem hiding this comment.
Hmm, that's a good point. I was thinking maybe we should keep the existing code and then throttle the $safeFind call, so you get sort of the both of best worlds. Thoughts?
A real optimization would involve parsing (using jquery.tokenize) the selector to watch for mutation involving potentially related elements
There was a problem hiding this comment.
throttle the $safeFind call
I don’t think that would match the exact arguments passed by that block at least half the time. But if you throttle the call and drop the optimization then it's fine.
However initialize is sync so throttling is not super straightforward.
I see two options for now:
- merge as is
- drop the optimization block (assuming this line covers the entire target just as well as the block)
Further improvements/optimizations almost certainly would be worse than just rewriting the library with proper testing around it, since at the core it's just:
- MutationObserver simply to detect document changes
- throttle calls
- avoid multiple callback calls for the same matched item (via WeakMap)
There was a problem hiding this comment.
I took the third way. The existing MutationObserver will be more performant
We already had an affordance for avoiding duplicate calls. That was required due to the MutationObserver running on subtree changes. An element could be visited as part of multiple mutations
src/vendors/initialize.js
Outdated
| @@ -153,6 +163,11 @@ msobservers.initialize = function (selector, callback, options) { | |||
| $(matches[i]).each(msobserver.callback); | |||
|
|
|||
| isMatchinInProgress = false; | |||
There was a problem hiding this comment.
This should be last or it might enter a loop
src/vendors/initialize.js
Outdated
| $(this).each(callback); | ||
| } | ||
| }; | ||
|
|
||
| var throttledCheckTarget = throttle( | ||
| function () { |
There was a problem hiding this comment.
Not sure why this non-arrow function as not blocked by the linter here 🤔
There was a problem hiding this comment.
Our lint rules are off for vendored code: src/vendors
pixiebrix-extension/.eslintignore
Line 3 in bb82140
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
initializeutility to re-check the entire selector. The original implementation only checked the mutated elements or the subtreeDiscussion
.isgets called on all modified/new elements. So theoretically the additional overhead shouldn't be much especially if using selectors with browser supportDemo
Checklist