New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #13434: native-API selector module #1180

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@gibson042
Member

gibson042 commented Feb 23, 2013

What's out:

  • 6 KB
  • attribute not equal selector
  • positional selectors (:first; :eq(n); :odd; etc.)
  • type selectors (:input; :checkbox; :button; etc.)
  • state-based selectors (:animated; :visible; :hidden; etc.)
  • :has(selector)
  • custom selectors
  • leading combinators (e.g., $collection.find("> *"))
  • reliable functionality on XML fragments
  • requiring all parts of a selector to match elements under context (e.g., $div.find("div > *") now matches children of $div)
  • matching against non-elements
  • reliable sorting of disconnected nodes
Fix #13434: native-API selector module
What's out:
* 6 KB
* attribute not equal selector
* positional selectors (:first; :eq(n); :odd; etc.)
* type selectors (:input; :checkbox; :button; etc.)
* state-based selectors (:animated; :visible; :hidden; etc.)
* :has(selector)
* custom selectors
* leading combinators (e.g., $collection.find("> *"))
* reliable functionality on XML fragments
* requiring all parts of a selector to match elements under context (e.g., $div.find("div > *") now matches children of $div)
* matching against non-elements
* reliable sorting of disconnected nodes
@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 23, 2013

Member

Also, QSA bug fixes is out. The lack of XML functionality could be a problem, and the context thing is confusing, especially if we want to move towards the new .find and .findAll methods in the future.

Member

timmywil commented Feb 23, 2013

Also, QSA bug fixes is out. The lack of XML functionality could be a problem, and the context thing is confusing, especially if we want to move towards the new .find and .findAll methods in the future.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 23, 2013

Member

The XML issues only manifest in a weird edge case, but could be resolved by bringing Sizzle's conditional into isXMLDoc.

Context, on the other hand, is something that I have strong reservations about because it's such a divergence. @dmethvin pointed out some other libraries that use purely native qSA, but I'm still not sure it's a good fit for us. Regardless, what better way to figure it out than with actual code?

Member

gibson042 commented Feb 23, 2013

The XML issues only manifest in a weird edge case, but could be resolved by bringing Sizzle's conditional into isXMLDoc.

Context, on the other hand, is something that I have strong reservations about because it's such a divergence. @dmethvin pointed out some other libraries that use purely native qSA, but I'm still not sure it's a good fit for us. Regardless, what better way to figure it out than with actual code?

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 23, 2013

Member

Ha, we could use that reasoning to do anything.

Member

timmywil commented Feb 23, 2013

Ha, we could use that reasoning to do anything.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 23, 2013

Member

To reiterate, though, the problem is that limiting scope is easy for simple selectors, but we can't do the same for compound selectors without duplicating Sizzle's tokenizer.

Member

gibson042 commented Feb 23, 2013

To reiterate, though, the problem is that limiting scope is easy for simple selectors, but we can't do the same for compound selectors without duplicating Sizzle's tokenizer.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 23, 2013

Member

I don't think we should release a native API that does not limit scope. This is precisely why .find and .findAll were added to the spec. QSA don't do it right.

Member

timmywil commented Feb 23, 2013

I don't think we should release a native API that does not limit scope. This is precisely why .find and .findAll were added to the spec. QSA don't do it right.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 23, 2013

Member

That said, I think we could release a plugin rather than a built-in build option and try to make it clear that the use of this API requires at least intermediate knowledge of querySelectorAll and at some point in the future will be obsolete.

Member

timmywil commented Feb 23, 2013

That said, I think we could release a plugin rather than a built-in build option and try to make it clear that the use of this API requires at least intermediate knowledge of querySelectorAll and at some point in the future will be obsolete.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Feb 23, 2013

Member

I'm down with a plugin, but it should be officially maintained by us and I'd like to be able to build jQuery from the jQuery repo with the plugin.

try to make it clear that the use of this API requires at least intermediate knowledge of querySelectorAll and at some point in the future will be obsolete.

That was always the point... expert understanding, use at own risk. I rarely use overly complex selectors in my client apps @bocoup—basically people like us are the target users for this functionality.

Member

rwaldron commented Feb 23, 2013

I'm down with a plugin, but it should be officially maintained by us and I'd like to be able to build jQuery from the jQuery repo with the plugin.

try to make it clear that the use of this API requires at least intermediate knowledge of querySelectorAll and at some point in the future will be obsolete.

That was always the point... expert understanding, use at own risk. I rarely use overly complex selectors in my client apps @bocoup—basically people like us are the target users for this functionality.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Feb 23, 2013

Member

Since you have to explicitly leave out Sizzle and include this code I would think it needs to be in the build process and the repo. I am okay with it having different semantics, we are not going to ship a built file with this in it and I suspect it isn't likely to be widely used, but it does give expert users a way to create a minimal build if they want.

Can we add the list of missing features in a block comment at the top of the selector-native.js file?

Member

dmethvin commented Feb 23, 2013

Since you have to explicitly leave out Sizzle and include this code I would think it needs to be in the build process and the repo. I am okay with it having different semantics, we are not going to ship a built file with this in it and I suspect it isn't likely to be widely used, but it does give expert users a way to create a minimal build if they want.

Can we add the list of missing features in a block comment at the top of the selector-native.js file?

}
return results;
},

This comment has been minimized.

@rwaldron

rwaldron Feb 23, 2013

Member

In a not-too-distant future...

[ ...Set( document.querySelectorAll("*") ) ]; // unique ;)
@rwaldron

rwaldron Feb 23, 2013

Member

In a not-too-distant future...

[ ...Set( document.querySelectorAll("*") ) ]; // unique ;)
@@ -39,10 +39,9 @@ module.exports = function( grunt ) {
"src/queue.js",
"src/attributes.js",
"src/event.js",
"src/selector.js",
{ flag: "sizzle", src: "src/selector-sizzle.js", alt: "src/selector-native.js" },

This comment has been minimized.

@dmethvin

dmethvin Feb 27, 2013

Member

This is a nice way to do it.

@dmethvin

dmethvin Feb 27, 2013

Member

This is a nice way to do it.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Feb 27, 2013

Member

I've captured the list of excluded features in a comment at the top of the new file.

Now, how to test this? Can't run it through full unit tests, but it's hilarious to watch it try. We've got custom selectors sprinkled all over the tests.

Member

dmethvin commented Feb 27, 2013

I've captured the list of excluded features in a comment at the top of the new file.

Now, how to test this? Can't run it through full unit tests, but it's hilarious to watch it try. We've got custom selectors sprinkled all over the tests.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 27, 2013

Member

I cleaned up a lot in 59f5adb... we could fully isolate the "jQuery only" assertions, but I'm not sure it's worth the effort.

Member

gibson042 commented Feb 27, 2013

I cleaned up a lot in 59f5adb... we could fully isolate the "jQuery only" assertions, but I'm not sure it's worth the effort.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Feb 28, 2013

Member

Landed at 1083f82.

Member

dmethvin commented Feb 28, 2013

Landed at 1083f82.

@dmethvin dmethvin closed this Feb 28, 2013

@antyrat

This comment has been minimized.

Show comment
Hide comment
@antyrat

antyrat Mar 2, 2013

this condition useless as will be always TRUE

antyrat commented on src/selector-native.js in edaa216 Mar 2, 2013

this condition useless as will be always TRUE

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 2, 2013

Owner

Incorrect; it will only be true when compareDocumentPosition returns a value with the least-significant bit set to indicate that the nodes have no common ancestor.

Owner

gibson042 replied Mar 2, 2013

Incorrect; it will only be true when compareDocumentPosition returns a value with the least-significant bit set to indicate that the nodes have no common ancestor.

This comment has been minimized.

Show comment
Hide comment
@antyrat

antyrat Mar 2, 2013

yeah, you're right, sorry, didn't noticed that there is only one amp symbol.

antyrat replied Mar 2, 2013

yeah, you're right, sorry, didn't noticed that there is only one amp symbol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment