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.