Skip to content
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 jQuery #12205: Miscellaneous pseudo selector issues #139

Closed
wants to merge 3 commits into from

Conversation

gibson042
Copy link
Member

Mostly stemming from 60c2549 (jQuery #12153).

http://jsfiddle.net/gXww6/1/

Net effect on jQuery master @ e07b444dfe87f26f0d6031920026142425505ad6

    258967      (+737)  dist/jquery.js                                         
     92590      (+103)  dist/jquery.min.js                                     
     33123       (+48)  dist/jquery.min.js.gz

Maybe someone else can do better?

@@ -91,6 +86,12 @@ var cachedruns,
compilerCache = {},
cachedSelectors = [],

// Return the original text of a tokenized selector
// We'd use `new String` instead of object literals, but jshint complains
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var cString = String; new cString("foo@); an alias get around that

@timmywil
Copy link
Member

I understand the dilemma, but having edge cases that don't work is more acceptable to me than using tokenize for everything. The regex may not be perfect, but I'm still not convinced it can't be close enough.

@gibson042
Copy link
Member Author

If we can learn anything from tickets, it's that people really do expect us to handle their complex queries.

Net effect on jQuery master @ e07b444dfe87f26f0d6031920026142425505ad6

    258701      (+471)  dist/jquery.js                                         
     92493        (+6)  dist/jquery.min.js                                     
     33102       (+27)  dist/jquery.min.js.gz

There's also a larger version that doesn't abandon selectorGroups in tokenize; let me know if you want to review that one.

@timmywil
Copy link
Member

We've always had edge cases for pseudo arguments. The ones that break in rc1 may differ from 1.7.2, but afaict, 1.8 has less. I don't know of any selector engine that doesn't split groups with a regex. Granted they don't all deal with complex arguments in pseudos so they may not have to deal with this, but we have to keep an eye on performance.

@gibson042
Copy link
Member Author

Traded size for speed with tokenization caching.

Net effect on jQuery master @ e07b444dfe87f26f0d6031920026142425505ad6

    258961      (+731)  dist/jquery.js                                         
     92604      (+117)  dist/jquery.min.js                                     
     33168       (+93)  dist/jquery.min.js.gz

@glebm
Copy link

glebm commented Aug 11, 2012

added :not(:first,:last) to the fiddle: http://jsfiddle.net/gXww6/3/

@timmywil
Copy link
Member

Thank you as always for your work!

Forgive me for writing my own version of your suggestion to use tokenize for grouping, but there are so many directions Sizzle can go. There are principles that I'd like Sizzle to uphold concerning style even if some edge case bugs sneak in at times. I didn't test your version, but the version now on master does maintain performance. As you know, there are still issues that need to be addressed, but I think with your help, we are in a great place. In other words, no offense meant in not landing this pull.

FYI, for performance testing, I am using several tests. Most are listed on the jQuery blog post about "The New Sizzle".

Open this in a browser window with a console already open to see the output:

http://jsfiddle.net/fS2rT/10/

Also, there are several jsperfs listed.

Looking forward to seeing your ideas on the rselector issue.

@timmywil timmywil closed this Aug 24, 2012
@mikesherov
Copy link
Member

@gibson042 @timmywil You guys are amazing.

@gibson042
Copy link
Member Author

No offense taken, @timmywil. I believe Sizzle to be much better for our sharpening each other's steel, so to speak. I'll work on the concepts I told you about, take advantage of your performance testing, and present the results.

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

Successfully merging this pull request may close these issues.

None yet

5 participants