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

Already on GitHub? Sign in to your account

proposing a configure() method #68

Merged
merged 10 commits into from Feb 7, 2012

Conversation

Projects
None yet
2 participants
Collaborator

rvagg commented Jan 14, 2012

As an alternative to directly setting nonStandardEngine, a configure() method that takes an options object that could be extended in the future (maybe..). NWMatcher uses a configure() so it seems like a good idea to standardise that; although it uses a USE_QSAPI key which I'm not so fond of so I've gone with NATIVE_QSA.

The select() method is set at configure time so there's no additional function layer to go through and read the boolean.

Updated the docs for this and remved reference to using the pseudos by disabling qSA support, as I said previously this shouldn't be necessary because of the try{}catch{} around qSA calls.

Also, I've adjusted the tests so that everything is run twice, once with qSA and again without qSA, should give really nice coverage of native and non-native in modern browsers. The native-qSA path needs testing cause there is still work done on the way there that can go wrong.

What think you?

Collaborator

rvagg commented Jan 16, 2012

I had a change of heart and went with camelCase useNativeQSA rather than the more Java-looking NATIVE_QSA (JavaScript is where I come to escape from Java).

Collaborator

rvagg commented Jan 16, 2012

Oh, and also, I'd like to put in a PR for adding matchesSelector in as default backend for is() where it's available, with a fall-back to non-native like we do with the try{}catch{} in selectCSS3() and selectCSS2qSA(). I have a small feature detect ready to go for it in Traversty: https://github.com/rvagg/traversty/blob/0a1ab78cae3a991f78631a3be60bfb03873b8c52/src/traversty.js#L14-18

This would mean it'd be logical to also have an additional config option: useNativeMatchesSelector / useNativeMatches / useNativeMS, whatever.

Collaborator

rvagg commented Jan 25, 2012

BTW, I'm thinking that we can pull out the all the IE8 specific CSS2 qSA stuff. Since we have the try/catch around qSA we could just loosen up the qSA detector (just check for existence, not CSS3 compliance) and let it fall through if native qSA doesn't support the query. Would remove a lot of cruft.

Owner

ded commented Jan 25, 2012

right on. agreed on that, 100%

@rvagg rvagg referenced this pull request Feb 5, 2012

Closed

complex pseudos fix #69

Collaborator

rvagg commented Feb 5, 2012

oooooooo yea, feels good to be removing code from Qwery rather than adding it, I should do this more often!

Sliced at least 30% off the simple benchmark suite across the board for IE8, current Qwery is on par with Sizzle for those but the new one is on par or quicker than NW (in IE8!). Mainly due to taking away 1 regex test and 1 regex match, plus doing away with your nonStandardEngine branch & apply code (sorry)--this accounts for a ~5% improvement in most other browsers.

Collaborator

rvagg commented Feb 6, 2012

Hey, I hope you don't mind this but I've done a tiny bit of rearranging, let me know if you want me to roll back any of these changes.

Moved pseudos.js to a new pseudos/ directory and made a package.json for you to publish qwery-pseudos. I've also put the actual source for both pseudos and mobile into their own src/ subdirectories and modified the Smoosh config so they build into their respective directories so the NPM publishables have the copyright included.

Some minor changes to pseudos to get it to work as an Ender package and as a standalone, plus some minor fixes picked up by JSHint.

Owner

ded commented Feb 6, 2012

hmmmm.... just reading the description, this is all fantastic news. Gotta be careful on the nonStandardEngine. Perhaps I should have labeled it as beta to begin with since I suppose the original intent was that it would change.

Owner

ded commented Feb 6, 2012

but to be open so you're not in the dark, i'm terribly busy today and really want to look over this one carefully. possibly later this evening

Owner

ded commented Feb 7, 2012

ok... here we gooooooooo

@ded ded added a commit that referenced this pull request Feb 7, 2012

@ded ded - style modifications
- bump version for new configure() method in #68
- update readme
96dedfc

@ded ded merged commit 452a183 into ded:master Feb 7, 2012

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