JQuery "find" replaced with more efficient "querySelectorAll" in SelectAdapter - #5775
JQuery "find" replaced with more efficient "querySelectorAll" in SelectAdapter#5775kevin-brown merged 2 commits into
Conversation
Sorry, something went wrong.
|
If you agree with my arguments and consider the PR appropriate, I'll gladly fix the tests. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@kevin-brown do you have any ideas why did the tests fail? I have the same message on my local computer: Running "qunit:all" (qunit) task Testing http://localhost:9999/tests/integration-jq1.html F >> PhantomJS timed out, possibly due to: >> - QUnit is not loaded correctly. >> - A missing QUnit start() call. >> - Or, a misconfiguration of this task. Meanwhile at the develop branch everything is OK, even locally. Maybe I need to (re)generate some files? Could you please help me understanding your workflow?) |
Sorry, something went wrong.
|
And when I open QUnit in browser I can see tests are passed: |
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
@kevin-brown you were absolutely right about iteration over NodeList, thanks a lot! Looks like the tests got fixed. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
@kevin-brown could you please take a look again? I've removed dist changes and modified current method of SelectAdapter (removed unnecessary check). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
* JQuery "find" replaced with more efficient "querySelectorAll" * Refactored to use Array.prototype.map Co-authored-by: Kevin Brown <kevin@kevin-brown.com>
This pull request includes a
The following changes were made
According 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 option tags.
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.