fix(perf): improve select performance fixes #702 #712
Conversation
lib/core/utils/select.js
Outdated
| if (!Array.isArray(context.include)) { | ||
| context.include = Array.from(context.include); | ||
| } | ||
| context.include.sort(axe.utils.nodeSorter); // ensure that the order of the include nodes is document order |
There was a problem hiding this comment.
We should enforce array type and sort the include as part of normalizeContext instead. That way this function doesn't have any side effects that can cause bugs later, and we don't have to sort more than once.
https://github.com/dequelabs/axe-core/blob/develop/lib/core/base/context.js#L72
There was a problem hiding this comment.
We would have to do it in the Context constructor because include is modified after normalizeContext. I can make that change.
lib/core/utils/select.js
Outdated
| } | ||
|
|
||
| return result.sort(axe.utils.nodeSorter); | ||
| if (context.include.length > 1 && hasOverlappingIncludes(context.include)) { |
There was a problem hiding this comment.
Instead if sorting here if the includes overlap, why not filter out any nested include and only run on the outer-most ones? I think if you do that you won't need to sort again.
lib/core/utils/qsa.js
Outdated
| * querySelectorAllFilter implements querySelectorAll on the virtual DOM with | ||
| * ability to filter the returned nodes using an optional supplied filter function | ||
| * | ||
| * @method querySelectorAll |
There was a problem hiding this comment.
Should be querySelectorAllFilter.
lib/core/utils/qsa.js
Outdated
| * @param {NodeList} domTree flattened tree collection to search | ||
| * @param {String} selector String containing one or more CSS selectors separated by commas | ||
| * @param {Function} filter function (optional) | ||
| * @return {NodeList} Elements matched by any of the selectors and filtered by the filter function |
There was a problem hiding this comment.
This returns an array, not a node list.
lib/core/utils/qsa.js
Outdated
| * @return {NodeList} Elements matched by any of the selectors and filtered by the filter function | ||
| */ | ||
|
|
||
| axe.utils.querySelectorAllFilter = function (domTree, selector, filter) { |
There was a problem hiding this comment.
You should have a few tests for this. Particularly to test the filter option.
lib/core/utils/qsa.js
Outdated
| ); | ||
| } | ||
|
|
||
| matchExpressions = function (domTree, expressions, recurse, parentShadowId, filter) { |
There was a problem hiding this comment.
Why did you need to pass the parentShadowId in separately? Looks to me like you could've just take that from domTree.
|
@WilcoFiers updated |
Improve the performance of
axe.utils.selectThis gets rid of the need for a sort in all instances except when there is an overlapping include context
Memoizes select so that subsequent calls to
axe.utils.selectfrom withinAudit.runwill return the cached results