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.

Sign up for GitHub

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

Jump to bottom

Optimize the Recursive Matching #68

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Optimize the Recursive Matching #68

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecb0c92
Optimize the Recursive Matching
Aug 30, 2018
158c771
Add left off method check
Aug 30, 2018
f09f004
Merge branch 'dev' into optimize-recursive-matching
May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/arrive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,12 @@ var Arrive = (function(window, $, undefined) {
callbacksToBeCalled.push({ callback: registrationData.callback, elem: node });
}

if (node.childNodes.length > 0) {
utils.checkChildNodesRecursively(node.childNodes, registrationData, matchFunc, callbacksToBeCalled);
if( !node.querySelectorAll ) continue;
var matchedDescendents = node.querySelectorAll(registrationData.selector);
Copy link
Owner

Choose a reason for hiding this comment

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

There are cases where querySlectorAll() won't match all elements. Consider this example:

<div class='parent-elem'>
  <div class='current-node'> <!-- Suppose node points to this element -->
    <div class='child'></div>
  </div>
<div>

Suppose arrive is called with following:

document.arrive('.parent-elem .child', (e) => {
  console.log(e);
})

querySelectorAll() won't match child element because we are calling it from .current-node element

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I think all my selectors are simpler than that (usually just looking for a certain class on a certain element). Sounds like we have a choice between:

  1. crashing on IE 11 if a large number of nodes change
  2. complex selectors not being supported in certain edge cases

Either kinda sucks.

I wonder if we could detect if a simple selector is being used and if so go native, if not, use the non-native strategy? That would seem to complicate things though. Maybe make it developer choice? Have an option to enable the more efficient route but with the caveat that certain complex selectors wouldn't be supported in certain edge cases. That would add less complexity and allow folks that don't have the complex selectors but do need to support IE to not need to use a fork of this library.

If either of these options sound like too much complication I would lean towards just closing this and I'll just have my own local fork so I can support IE 11. Hopefully in Oct 2025 when IE 11 is EOL we can move back to the mainline fork. :)

Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we could detect if a simple selector is being used and if so go native, if not, use the non-native strategy?

This seems like a good option.

Maybe make it developer choice? Have an option to enable the more efficient route but with the caveat that certain complex selectors wouldn't be supported in certain edge cases.

Hmm. This option would be a bit hard to explain to normal users. I wonder if we can detect simple selector using a regex maybe? I think in a lot of cases users are using simple selectors and if we can detect it intelligently, it would make significant performance improvements for a lot of users.

Copy link
Author

Choose a reason for hiding this comment

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

I just checked and all of my selectors are a single ID or class on an element. Something like #users_index. The only exception is I use the comma to specify multiple selectors on a the same line (but again we are just looking for a single class or ID for each sub-selector). For example: .form-checkbox, .form-radio

It seems if we take the selector, split on comma, trim any whitespace before and after each split part and the check to ensure each split part does not still have a space then that would satisfy all my needs. If someone later comes up with a more complicated selector that does not satisfy that criteria but still could use the native search we could always augment but that would be a good start.

If that sounds good I'll update this PR to use this strategy.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, this seems like a good start. We can intelligently handle cases where attribute value contains spaces e.g. ‘.test[title=“some title”]’ but this does not necessarily have to be part of this pull request, we can handle it later.

Also, please send the pull request to dev branch. I’ll merge to master after release.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this project is still active anymore but finally had a few hours to look into seeing about closing this issue out.

Turns out my original code should account for the scenario you uncovered. querySelectorAll does not work like jQuery.find. See this note on MDN for more info.

This was news to me also. I was trying to write a test case where if I took the native path on a nested selector it would fail but if I took the original code path it would pass. But since querySelectorAll doesn't behave like we both assumed it was actually passing either way and my original code will handle the case you describe above.

Given this, if you are still interested this PR should be good to merge. I have re-pointed the PR to the dev branch.

for (var j=0, descendent; (descendent = matchedDescendents[j]); j++) {
if (matchFunc(descendent, registrationData, callbacksToBeCalled)) {
callbacksToBeCalled.push({ callback: registrationData.callback, elem: descendent });
}
}
}
},
Expand Down Expand Up @@ -467,4 +471,4 @@ var Arrive = (function(window, $, undefined) {

return Arrive;

})(window, typeof jQuery === 'undefined' ? null : jQuery, undefined);
})(window, typeof jQuery === 'undefined' ? null : jQuery, undefined);