search

uzairfarooq / arrive

Watch for DOM elements creation and removal
MIT License
869 stars 99 forks source link

Optimize the Recursive Matching #68

Open eric-hemasystems opened 6 years ago

eric-hemasystems commented 6 years ago

I was trying to use arrive in combination with the Turbolinks library and the webpage was crashing about every other page change in IE 11.

I found this happened even when the "arrive" callback registered was a no-op function (i.e. function() {}) and a non-existent node. So the crash wasn't due to something in what we were doing with arrive but due to something arrive was doing itself.

Since the entire tab was crashing it made it difficult to debug so I started just using the MutationObserver API directly slowly building up to some of what arrive does. Early in the process I found that if I spent too long in the MutationObserver callback I would get this crash.

This made sense as Turbolinks replaces the entire body meaning that the entire DOM is traversed looking for matches. I know Stimulus (also from the Rails universe) is often paired with Turbolinks and Stimulus operates via the MutationObserver. This caused me to investigate how they found matching elements without crashing IE when a large number of elements change.

Found the answer at:

https://github.com/stimulusjs/stimulus/blob/8cb58d1f8a4ac83a875c7a1fdc7a28f21869efd6/packages/%40stimulus/mutation-observers/src/attribute_observer.ts#L52-L56

They use matchElement like arrive does on the node actually reported by the mutation observer. But then to search the child nodes it uses querySelectorAll. This bring the matching and recusing into native code which is much faster.

For arrive I decided to just use querySelectorAll as an initial filter and to handling the recusing. Once we get the list then it still runs through the matchFunc so that it still does the tracking that avoids repeating for the same element.

Even if not using Turbolinks (or similar library) this should optimize any site using arrive where a large number of nodes change.

eric-hemasystems commented 3 months ago

Just adding a note to this. I updated this branch to the latest from the dev branch since I was testing out this optimization on another app so it should be ok to merge.

Regarding the discussion of the impl, I think when we last left it off the existing code already addressed your concern so it should be 100% compatible with the existing impl.