Skip to content

fix(ts): use lru-cache for local layer, deterministic key sort, safety docs - #140

Open
lan17 wants to merge 1 commit into
mainfrom
lev/ts-review-fixes
Open

fix(ts): use lru-cache for local layer, deterministic key sort, safety docs#140
lan17 wants to merge 1 commit into
mainfrom
lev/ts-review-fixes

Conversation

@lan17

@lan17 lan17 commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up hardening from the review of #137 (TypeScript GCache package). No public API changes; the behavior changes all move toward correctness/safety.

Changes

  • Local cache now uses lru-cache instead of a hand-rolled Map. The previous implementation evicted FIFO (insertion order) and bounded size per use case (worst-case memory maxSize × number_of_use_cases). It is now true LRU eviction with a single global instance keyed by URN, so localMaxSize is a real global bound.
    • TTL remains enforced via Date.now() (consistent with the Redis layer and mockable under fake timers); lru-cache handles only LRU + max-size. lru-cache's own TTL clock uses performance.now(), which the test suite's fake timers don't mock.
    • Values are boxed ({ value, expiresAtMs }) so an undefined fallback result stays cacheable (lru-cache.set(k, undefined) is an alias for delete).
  • Deterministic cache keys: normalizeArgs sorts argument names by UTF-16 code unit instead of localeCompare (which is locale/ICU-version dependent). Prevents the same logical call from producing different URNs on different hosts, which would silently lower the Redis hit rate.
  • Docs/safety:
    • flushAll() JSDoc + README WARNING: it issues Redis FLUSHALL, which wipes the entire Redis instance, not just keyPrefix keys.
    • README IMPORTANT: the local layer is not supported for sensitive/strong invalidation (trackForInvalidation only consults the Redis watermark; local hits are not synchronously invalidated, and a value can be written to local while the remote layer is ramped down).

Dependency

Adds lru-cache@^10. Pinned to v10 (not v11) to preserve the package's declared engines.node: ">=18.17"lru-cache@11 requires Node 20 || >=22. If you'd rather adopt v11 and drop EOL Node 18, that's a one-line bump to both lru-cache and engines.node.

Testing

  • pnpm install --frozen-lockfile
  • pnpm ts:gcache:typecheck — clean
  • pnpm ts:gcache:test — 73 passed, coverage 97.7% statements / 92.12% branches / 97.65% functions / 97.84% lines
  • pnpm ts:gcache:build — esm + cjs + dts

Deferred (non-blocking)

Review items intentionally left for later: prom-client loads even with metrics: false; CI on: push + pull_request double-runs; redundant workspaces field in root package.json; get timer includes config-provider latency; no direct test asserting LRU eviction.

🤖 Generated with Claude Code

…y docs

Follow-up hardening from the #137 review:

- Replace the hand-rolled local cache (FIFO, per-use-case max size) with
  lru-cache: true LRU eviction and a single global bound keyed by URN. TTL
  stays enforced via Date.now() (consistent with the Redis layer and mockable
  in tests); values are boxed so undefined results stay cacheable.
- normalizeArgs sorts by UTF-16 code unit instead of localeCompare, so cache
  keys are byte-for-byte identical across processes and ICU versions.
- Document that flushAll() issues Redis FLUSHALL (whole-instance), and that the
  local layer is not supported for sensitive/strong invalidation.

Pinned lru-cache to ^10 to preserve the package's Node >=18.17 floor (v11
requires Node 20+).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lan17
lan17 force-pushed the lev/ts-review-fixes branch from 0d6fc47 to 4e8c97e Compare June 17, 2026 03:51
@lan17
lan17 enabled auto-merge (squash) June 17, 2026 03:54
@galileo-automation

Copy link
Copy Markdown
Contributor

No activity for 30 days — this PR will be closed in 5 days unless updated.

1 similar comment
@galileo-automation

Copy link
Copy Markdown
Contributor

No activity for 30 days — this PR will be closed in 5 days unless updated.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants