Bug Tracker

Modify

Ticket #447 (closed bug: fixed)

Opened 7 years ago

Last modified 7 years ago

Refactoring of the filter function

Reported by: andrea ercolino Owned by:
Priority: minor Milestone: 1.1a
Component: core Version: 1.1a
Keywords: refactoring, filter Cc:
Blocking: Blocked by:

Description

I've refactored the "filter" core function of jQuery, merging my Chili parser into it. Now it should be cleaner, faster and tighter.

Main changes:

(*) a changed parsing engine where only 1 regexp match is executed on any call (there were a bunch more)

(*) a new preprocessing phase which builds one regexp out of the configuration object (is executed only on the first call, and is cached for later use)

(*) a changed configuration object (there were 2), which holds the regular expressions together with relative checks on the elements

(*) a tighter grammar, preserving its ease of definition

(*) comments, meaningful names

(*) fixed many bugs in the regexps

 http://jquery.com/discuss/2006-November/018337/  http://jquery.com/pipermail/discuss_jquery.com/attachments/20061128/2974428c/utf-8qfilter-refactoring.txt

Change History

comment:1 Changed 7 years ago by andrea ercol

Please change Component: ajax to core. Thanks!

comment:2 Changed 7 years ago by brandon

  • Component changed from ajax to core

comment:3 Changed 7 years ago by andrea ercol

No feedback?

comment:4 Changed 7 years ago by andrea ercol

WARNING: This Wiki is messing up the regexps in this message


In my refactoring, there is a bug in this line:

step.len = 1 + (exp.replace(/
|\(/g, "").match(/((?!?)/g)
"" ).length;

the correct line should read:

step.len = 1 + (exp.replace( /\./g, "%" ).replace( /[.*?]/g, "%" ).match( /((?!?)/g )
[]).length

I posted an  article about this issue.

comment:5 Changed 7 years ago by joern

Does this refactoring also handle the problems described in bug #453 ( http://jquery.com/dev/bugs/bug/453/)? As not() uses filter(), fixing the problem in filter() should also fix not()...

I also wonder if you can find a way to replace/remove the old regexes completely. Currently that block at the end looks quite ugly.

comment:6 Changed 7 years ago by andrea

As Renato said in #453, the problem is that jQuery.filter does not support multiple selectors, because it matches from the beginning of the target, does the filtering, erases the recognized token in the target, and then starts another loop on the changed target. And as soon as the first selector is completely recognized (in n loops), the target becomes something that starts with a comma, which does not match anything, and filter ends.

A solution to #453 and the more general "multiple selectors" issue should be implemented inside $().not() and $().filter(). It shouldn't be too hard to explode on the comma in the target, filter by any piece, and finally add all up.

As for the old regexps at the bottom of the refactoring, surely they can be completely removed if the new interface in case of failure is considered acceptable. It's also very simple: remove all the "if( typeof matches == "undefined" ) {...}".

Note that I'm talking about interface in case of failure here, meaning that in case of success (a match is found between the regexps and the selector) the interface is preserved (filter works as it is supposed to). So the problem is really marginal, and I cannot think of someone in need of the old inconsistent interface. I provided that last check only to be rigorous about the refactoring.

The OLD interface in case of failure is: jQuery.filter( target, result ) = { r: [], t: changed_target }

The NEW interface in case of failure is: jQuery.filter( target, result ) = { r: result, t: target }

comment:7 Changed 7 years ago by john

  • Priority changed from major to minor
  • Version 1.1 deleted

I think we're going to hold off on a complete refactor of the filter function, for now. (Considering that a complete speed overhaul was just finished.)

I recommend that you bring the refactoring up on the dev mailing list:  http://jquery.com/mailman/listinfo/dev_jquery.com

Some things that would help would be to point out any decreases in file size and any increases in speed that your patch provides.

comment:8 Changed 7 years ago by john

  • Status changed from new to closed
  • Version set to 1.1
  • Resolution set to fixed
  • Milestone set to 1.1

Ok, I've integrated the IE speed fixes that you mentioned - and its helped a ton. I'm going to hold off on a complete re-org of filter for now, but your code is definitely pointing me in the right direction. Thanks again for all your help.

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.