Remove querySelector and querySelectorAll from shared/dom.js

[kristoferbaxter]

Hello,

I noticed when exploring examples using HTMLSelectElements that querySelector and querySelectorAll was used to find the options checked. I'm working on a DOM implementation that doesn't currently support these APIs and made changes locally to continue along.

First, this change uses selectedIndex when only a single value is allowed in a HTMLSelectElement. selectedIndex has been present since DOM level 1 and should provide a reasonable replacement. (https://www.w3.org/TR/REC-DOM-Level-1/level-one-html.html#ID-94282980)

Secondly, when multiple values are possible, this change moves to iterating over the options and creating an array of values for selected options.

Am slightly unfamiliar with the structure of the project, and wasn't sure where to add tests regarding the performance of these implementations. Happy to do so or make other changes if helpful.

Total change adds 23 bytes to uncompressed (but minified) output of an application leveraging both functions.

[codecov-io]

Codecov Report

Merging #1196 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1196   +/-   ##
=======================================
  Coverage   91.63%   91.63%           
=======================================
  Files         127      127           
  Lines        4567     4567           
  Branches     1503     1503           
=======================================
  Hits         4185     4185           
  Misses        160      160           
  Partials      222      222

---

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 8aaf92a...e6db586. Read the comment docs.

[Conduitry]

I'm not sure this is something we'd want to do. We do have a legacy: true compilation flag that makes a few code changes for IE9's benefit - but doing things that cater to environments that don't support querySelector/querySelectorAll feels like a road we don't really want to go down. QS/QSA support has been good for quite some time. This is of course up to Rich though.

[kristoferbaxter]

Wouldn't be an issue if you choose not to accept. I made a small (terrible and untrustworthy) performance test here if you're interested.

https://pumped-look.glitch.me/

Chrome 64 Mac

test-1, selector: 0
test-1, dom v1: 0
test-2, selector: 2.300000000104774
test-2, dom v2: 0
test-2, dom v1: 1.8999999956577085

Safari 11 Mac

test-1, selector: 0
test-1, dom v1: 0
test-2, selector: 2
test-2, dom v2: 0
test-2, dom v1: 1.0000000000002274

[Rich-Harris]

We used to use selectedOptions but it turned out that wasn't IE-friendly, so we switched to querySelector. In principle this could work if it was equally compatible.

I'm not too concerned about any performance difference, and the filesize difference is probably acceptable given that it doesn't affect anyone who isn't using selects (and the difference is negligible if it's not a multiple select). That said @Conduitry makes a good point about choosing not to support environments with QS/QSA, as it could prove to be a tricky constraint in future.

Out of curiosity have you found any other incompatibilities with your DOM implementation, or is that the only one so far?

[kristoferbaxter]

That said @Conduitry makes a good point about choosing not to support environments with QS/QSA, as it could prove to be a tricky constraint in future.

Agree this is a tricky constraint. I haven't found a lot of use for QS/QSA in applications like those written using Svelte. When you have a direct reference to DOM nodes you generate, the need for query selection against them is lessened.

However, it's also just as likely others have found great success using them in similar applications.

Out of curiosity have you found any other incompatibilities with your DOM implementation, or is that the only one so far?

This is the only one so far, DOMTokenList and Element.querySelector[All]() are the last remaining large pieces that remain incomplete. DOMTokenList is used frequently for Element.classList and Element.className can leverage it via set Element.classList.value so it's implementation is required.

A suggestion perhaps: Strive to not use querySelector when a DOM Level 1 API exists and does the same operation – but use it when it makes sense otherwise.

[Rich-Harris]

worker-dom will be gaining support for element.querySelector[All] in the near future (thanks Kris!) so I'm going to close this