feat(ByRole): Check only elements that could be a match

[ph-fritsche]

What:

Let querySelectorAll pre-filter elements if no normalizer is passed before running our own filters on them in queryAllByRole.

Why:

byRole is slow.

How:

If role parameter is a string, we can predetermine which elements might match our filter functions.
By dismissing other elements in querySelectorAll we reduce the time spent in our filter functions.

Checklist:

• [N/A] Documentation added to the
  docs site
• [N/A] Tests
• [N/A] TypeScript definitions updated
• Ready to be merged

Additional notes

I ran getByAllRoles(container, 'table') on the example document for role-helpers.
The avg time spent dropped from 9.5ms to 4.2ms.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a592dba:

Sandbox	Source
react-testing-library-examples	Configuration

[codecov]

Codecov Report

Merging #1046 (a592dba) into main (fb6f2d5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1046   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          919       927    +8     
  Branches       283       288    +5     
=========================================
+ Hits           919       927    +8     

Flag	Coverage Δ
node-12	100.00% <100.00%> (ø)
node-14	100.00% <100.00%> (ø)
node-16.9.1	100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/queries/role.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb6f2d5...a592dba. Read the comment docs.

[MatanBobi]

If I'm not mistaken here, the thing is that we have roles that are implicitly defined (by the elements) and we'll probably lose those if we filter using explicit role selector won't we?

[eps1lon]

How did you verify that this improves performance? Please share the results so that we can judge whether this optimization is worth the added code.

[ph-fritsche]

If I'm not mistaken here, the thing is that we have roles that are implicitly defined (by the elements) and we'll probably lose those if we filter using explicit role selector won't we?

Handled here:

dom-testing-library/src/queries/role.js

Lines 190 to 192 in 77d3291

	const roleRelations = roleElements.get(role)
	const implicitRoleSelectors =
	roleRelations && new Set(Array.from(roleRelations).map(({name}) => name))

The matching on attributes with implicit roles as performed by the filter could not be handled by querySelectorAll because the query runs on content attributes and we support querying roles per IDL attributes. Therefore I could not skip the following filter function completely.

But narrowing the selection according to the element types which might be matched is possible.

[ph-fritsche]

How did you verify that this improves performance? Please share the results so that we can judge whether this optimization is worth the added code.

https://github.com/ph-fritsche/dom-testing-library/blob/a63a64f2bc73bfb89feba576eca7fe014ef9d2b8/src/__tests__/role-perf.js

[eps1lon]

So this was only tested in JSDOM? Would be nice if you could share a fixture that we can run with this PR in any environment and post the findings you have.

[ph-fritsche]

In the browser just the queryAllByRole:

old avg: 3.1051999999284745 vs new avg: 0.20780000004172325

[ph-fritsche]

old avg: 3.1051999999284745 vs new avg: 0.20780000004172325

@eps1lon Do you consider this enough evidence that adding code for creating a narrower selector is more efficient than running the following filter on all elements in the tree?

[eps1lon]

Very helpful test fixture, thanks!

Something did set me off about this implementation. I think we tested this in the past and didn't see an significant gains.

Fiddled around with it a bit and it turns out that it can be slower for small DOM trees. Though for small DOM trees we're already fast enough that a slight slowdown (-~10%) is totally fine compared to having an order of magnitude faster queries for large DOM trees where perf really matters.

In the future, please include this work up front. If you already tested it locally, then sharing the work is in your interest as well for future reference. Perf improvements should just never go in untested.

[github-actions]

🎉 This PR is included in version 8.9.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀