feat(jqLite): use querySelectorAll instead of getElementsByTagName in jqLite.find

[jankuca]

Addresses #3586

[mary-poppins]

Thanks for the PR!

• Contributor signed CLA now or in the past
  • If you just signed, leave a comment here with your real name
• PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[IgorMinar]

is there a queryselectorall polyfill? I found https://gist.github.com/branneman/1200441

qsa is supported on IE8+ so that's good

I'm happy to merge this in after 1.2

[jankuca]

The gist is just using the plain old Sizzle, just like jQuery does. Do we want to include Sizzle somehow or should we leave this up to the app to handle?

On Thursday, August 15, 2013 at 10:33 AM, Igor Minar wrote:

is there a queryselectorall polyfill? I found https://gist.github.com/branneman/1200441
qsa is supported on IE8+ so that's good
I'm happy to merge this in after 1.2

—
Reply to this email directly or view it on GitHub (#3598 (comment)).

[ledbettj]

👍 Thanks for implementing.

[leesei]

Sizzle may be too large (minified@18k) for AngularJS.

[hallister]

18k seems expensive, given that the only browsers that don't support it is IE 6 and 7. And that problem is solved by calling element.getElementsByTagName() when element.querySelectorAll() doesn't exist.

[mgol]

If jqLite continues to grow, at one point it might be better to replace it with custom jQuery 2.x build. You can even exclude Sizzle there! jQuery handles lots of cross-browser problems so that would be a plus. I'd be monitoring the situation about when it would be a good moment to switch (as jqLite grows and jQuery gets more & more componentized, we e.g. switched internally to AMD recently).

I am a jQuery Core team member, though, so I might be a little biased. :)

[expando]

return (element.querySelectorAll || element.getElementsByTagName).call(element, selector);

The conditional will cause silent errors for IE<8 if the selector is not a tag name. It's better to only use querySelectorAll so that users on IE<8 get an error, rather than not see an error, but don't get the expected result either.

I'd rather have conditional code blocks for IE<8 if they work as expected, even though we don't official support it, rather than conditional code blocks that may or may not work everytime.

Or alternatively, explicitly state that the function only accepts tag names and not full CSS queries. In that case, the function would run faster if it used getElementsByTagName over querySelectorAll under certain scenarios and would be the same in the rest of the scenarios.
http://jsperf.com/access-to-nodes-via-queryselectorall-vs-getelementsbyta/6

[hswolff]

Instead of Sizzle perhaps look at https://github.com/ded/qwery as a querySelectorAll polyfill?

I only just came across it (don't know anything about it), however it looks much slimmer than the polyfill linked to above.

[petebacondarwin]

@mzgol - I am pretty sure that at some point we will do exactly that. The question is as you say when the optimal moment arrives.

[clakech]

👍 would love to get this in ng

[hsablonniere]

If we get this in 1.2 this way we'll have this problem #4120. Don't forget to verify if there's no text nodes.

[teddyhwang]

👍

[nosideeffects]

👍

[IgorMinar]

@mzgol we tried building a custom jquery2 build but it was still huge.

[IgorMinar]

@mzgol quite frankly, I'd love to throw jqLite away if we could get a jQuery build of comparable size and functionality.

[malsup]

@IgorMinar In the meantime, why not just merge this pull request? It makes "find" significantly more useful.

[dtabuenc]

+1

[OverZealous]

+1

[malsup]

+1

[sbruchmann]

+1

[mofas]

+1

[jorgenfb]

+1

[tbertenshaw]

+1

[codef0rmer]

+1

[wilsonjackson]

This is a great addition, but I'm a little concerned about the unintuitive scoping behavior of querySelectorAll. This would be easy to patch for browsers that support the :scope pseudo element, but sadly that leaves IE8 out in the rain — and maybe more, I haven't researched :scope support thoroughly.

[just-boris]

+1

[GabrielDelepine]

+1

[flekschas]

+1

[IgorMinar]

fixing this properly is non trivial and won't happen in 1.3. moving this back to backlog.

[dhzhuo]

+1

[ilanbiala]

Any plans on merging this in time for 1.4 or 2.0?

[caitp]

@ilanbiala --- the main reason we don't is because we wouldn't be able to provide the expected results (although we could provide very similar results to jquery 2.x).

There are polyfills available for some of the missing features, (like https://github.com/termi/CSS_selector_engine), so it's probably possible for us to do it.

Leave that up to @petebacondarwin though

[ilanbiala]

@caitp thanks for the update.

[mgol]

If we were to use an external CSS selector engine, we might as well use the
most popular & battle-tested one in the world: Sizzle that is included in
jQuery... 18 KB minified. The one @caitp linked to is 10 KB, so also not
extremely small.

BTW, Sizzle will be dropping support for browsers not supported by jQuery
which will decrease its size.

I feel that escapes jqLite's goal, though - if we want to be more
feature-complete & stable even if that sacrifices the size, we have jQuery
for that.

[mgol]

Correction: since only gzipped sizes matter; here they are:

Sizzle: 6.9 KB
CSS_selector_engine: 4.6 KB

And note that the next Sizzle version will be smaller.

[bzitzow]

+1

[starzou]

+1

[daniellmb]

+1

[caitp]

fixing this properly is non trivial and won't happen in 1.3. moving this back to backlog.

I think it's time for this one to be closed. we can take another attempt at this in the future (with the next level of DOM), but I don't think there's much hope for the aspirations of this right now.