JQuery "find" replaced with more efficient "querySelectorAll" in SelectAdapter #5775
Conversation
|
If you agree with my arguments and consider the PR appropriate, I'll gladly fix the tests. |
|
This is something I'm willing to entertain, and the timing is great since we're about to start dropping legacy support for IE. The tests need to be fixed, as you've noted, but this looks like a solid start. |
|
@kevin-brown do you have any ideas why did the tests fail? I have the same message on my local computer: Meanwhile at the Maybe I need to (re)generate some files? Could you please help me understanding your workflow?) |
|
And when I open QUnit in browser I can see tests are passed: |
src/js/select2/data/select.js
Outdated
|
|
||
| data.push(option); | ||
| }); | ||
| for (var index in selectedElements) { |
There was a problem hiding this comment.
Is there a reason why you chose to use for ... in here? MDN explicitly recommends against using it for a NodeList, which is what is being returned by querySelectorAll.
src/js/select2/data/select.js
Outdated
| data.push(option); | ||
| }); | ||
| for (var index in selectedElements) { | ||
| if (selectedElements.hasOwnProperty(index)) { |
There was a problem hiding this comment.
I suspect this is allowing non-DOMElement properties of the NodeList to make their way through in tests which is causing the test failures we are seeing in CI. I would recommend falling back to a traditional index-based for loop which should be fine for this situation and should ensure that we only ever try to call .item on a DOM element.
|
@kevin-brown you were absolutely right about iteration over |
kevin-brown
left a comment
There was a problem hiding this comment.
The changes in general look good, but the changes to the dist files need to be removed from the pull request. We recompile those files before every release separately, since otherwise we would run into a lot of merge conflicts within the files during reviews.
I also have one question about a guard that was included.
|
@kevin-brown could you please take a look again? I've removed dist changes and modified |
|
This looks good and it'll land in the next release (4.1.0) which is coming at some yet-to-be-determined date. Right now the status checks are failing but that appears to be because of a GitHub Actions issue. I'll look into that later, but it shouldn't impact this. |
This pull request includes a
The following changes were made
SelectAdapter'scurrentmethod replaced expensive call tojQuery.findwith more efficient call toDOMElement.querySelectorAllAccording to W3C Recommendation 06 November 2018,
which is applicable for our case.
Here is an example illustrating the problems of jQuery.find method: https://jsfiddle.net/vyshkant/nzr4kp8u/12/
CAUTION: an execution of the example might take about 2 minutes (which actually illustrates the degradation of jQuery.find with increasing the number of
optiontags.In my personal case I have a middle-size db which includes about 20 000 of options and it is impossible to use Select2 because of the timing.