Conversation
…, setWithEvicted cleanup
- Add removeFromList definition with all 4 prev/next cases - Expand moveToEnd formula and document 'only node' edge case - Fix TTL validity invariant to handle expiry=0 when ttl=0
- Clear prev/next pointers in delete() and evict() to prevent memory leaks - Fix entries() and values() to not pollute LRU order (access items directly instead of calling get()) - Fix entries() and values() returning stale data with TTL (direct item access instead of get() which can delete expired items) - Fix expiresAt() to return expiry for expired-but-not-yet-deleted items - Optimize setWithEvicted() to avoid redundant has()+set() calls - Remove dead code in moveToEnd() (unreachable edge case) - Remove obsolete moveToEnd edge case test
🤖 Augment PR SummarySummary: This PR refreshes documentation/tooling and tweaks the LRU internals around TTL-awareness and iteration helpers. Changes:
Technical notes: The behavioral changes primarily affect TTL visibility via 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
src/lru.js
Outdated
| const key = keys[i]; | ||
| result[i] = [key, this.get(key)]; | ||
| const item = this.items[key]; | ||
| result[i] = [key, item !== undefined ? item.value : undefined]; |
There was a problem hiding this comment.
entries() now reads item.value directly and doesn’t apply the TTL expiry check that get() performs, so an expired-but-not-yet-accessed entry could still be surfaced. This can make entries()/values() disagree with has()/get() for TTL caches.
Severity: medium
Other Locations
src/lru.js:452
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
src/lru.js
Outdated
|
|
||
| if (this.has(key)) { | ||
| this.set(key, value, true, resetTtl); | ||
| if (item !== undefined) { |
There was a problem hiding this comment.
setWithEvicted() treats this.items[key] !== undefined as “exists”, so an expired entry that’s still stored will be updated in-place and (when resetTtl is false) can remain immediately expired after the update. That makes re-inserting a key after TTL has elapsed behave unexpectedly compared to inserting a missing key.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
src/lru.js
Outdated
| @@ -333,16 +330,25 @@ export class LRU { | |||
| */ | |||
| setWithEvicted (key, value, resetTtl = this.resetTtl) { | |||
There was a problem hiding this comment.
setWithEvicted() now returns an evicted object with { key, value, expiry }, but the JSDoc above still describes the evicted shape as including prev/next. This mismatch can confuse consumers relying on the inline documentation.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| const cache = lru(max, ttl = 0, resetTtl = false); | ||
| const cache = lru(); // 1000 items, no TTL | ||
| const cache = lru(500); // 500 items, no TTL |
There was a problem hiding this comment.
The factory-function example redeclares const cache multiple times in the same snippet, which will throw if copied verbatim. This makes the “parameter combinations” section hard to use as-is.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| cache.clear(); | ||
| // Get values for specific keys | ||
| const values = cache.values(["users:3", "users:1"]); | ||
| // ['Carol', 'Alice'] - maintains LRU order |
There was a problem hiding this comment.
values(["users:3", "users:1"]) returns values in the provided key order (the implementation iterates the keys array), so the comment about “maintains LRU order” looks incorrect. A similar ordering claim appears in the entries(["c", "a"]) example.
Severity: low
Other Locations
docs/API.md:180
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
tests/unit/lru.js
Outdated
| import {LRU, lru} from "../../src/lru.js"; | ||
| import {strict as assert} from "assert"; | ||
| import {describe, it, beforeEach} from "node:test"; | ||
| import assert from "node:assert"; |
There was a problem hiding this comment.
Importing node:assert means assert.equal/assert.deepEqual are the loose variants, whereas the previous assert/strict behavior enforced strict equality under the same method names. This can reduce test sensitivity to type mismatches.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
1 participant
No description provided.