Side navigation
#15222 closed bug (worksforme)
Opened August 31, 2014 04:46AM UTC
Closed September 01, 2014 01:29PM UTC
cacheLength is ignored in Sizzle's createCache()
Reported by: | nessup | Owned by: | |
---|---|---|---|
Priority: | high | Milestone: | None |
Component: | selector | Version: | 1.9.1 |
Keywords: | sizzle, cache, cachelimit | Cc: | |
Blocked by: | Blocking: |
Description
It appears that elements are added to Sizzle caches regardless of whether the cache limit has been exceeded. This bug affects 1.9.1 and appears to also affect 2.1.1. Consider:
function createCache() { var cache, keys = []; return (cache = function( key, value ) { // Use (key + " ") to avoid collision with native prototype properties (see Issue #157) if ( keys.push( key += " " ) > Expr.cacheLength ) { // Only keep the most recent entries cache[ keys.shift() ] = null; } return (cache[ key ] = value); }); }
While the keys array will always be within the cache limit, the actual cache instance will store the cached object regardless of whether the limit has been exceeded.
Barring verbosity, I think that this method should be written as follows:
function createCache() { var cache, keys = []; return (cache = function( key, value ) { // Use (key + " ") to avoid collision with native prototype properties (see Issue #157) if ( keys.push( key += " " ) > Expr.cacheLength ) { // Only keep the most recent entries delete cache[ keys.shift() ]; return value; } else { return (cache[ key ] = value); } }); }
Notice that, now, the cache variable only increases in length when key.length <= Expr.cacheLength. I think this is truer to the intent of cacheLimit. However, I'm not sure whether return value;
makes sense in the affirmative branch of the control flow, and need assistance with this particular point.
Am I correct here? Or is cache length only meant to limit the size of the keys array? If so, why?
Attachments (0)
Change History (1)
Changed September 01, 2014 01:29PM UTC by comment:1
resolution: | → worksforme |
---|---|
status: | new → closed |
This is being handled at https://github.com/jquery/sizzle/issues/281.