fix(ts): use lru-cache for local layer, deterministic key sort, safety docs - #140
Open
lan17 wants to merge 1 commit into
Open
fix(ts): use lru-cache for local layer, deterministic key sort, safety docs#140lan17 wants to merge 1 commit into
lan17 wants to merge 1 commit into
Conversation
ShuaiShao93
approved these changes
Jun 17, 2026
…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
force-pushed
the
lev/ts-review-fixes
branch
from
June 17, 2026 03:51
0d6fc47 to
4e8c97e
Compare
lan17
enabled auto-merge (squash)
June 17, 2026 03:54
Contributor
|
No activity for 30 days — this PR will be closed in 5 days unless updated. |
1 similar comment
Contributor
|
No activity for 30 days — this PR will be closed in 5 days unless updated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
lru-cacheinstead of a hand-rolledMap. The previous implementation evicted FIFO (insertion order) and bounded size per use case (worst-case memorymaxSize × number_of_use_cases). It is now true LRU eviction with a single global instance keyed by URN, solocalMaxSizeis a real global bound.Date.now()(consistent with the Redis layer and mockable under fake timers);lru-cachehandles only LRU + max-size. lru-cache's own TTL clock usesperformance.now(), which the test suite's fake timers don't mock.{ value, expiresAtMs }) so anundefinedfallback result stays cacheable (lru-cache.set(k, undefined)is an alias fordelete).normalizeArgssorts argument names by UTF-16 code unit instead oflocaleCompare(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.flushAll()JSDoc + READMEWARNING: it issues RedisFLUSHALL, which wipes the entire Redis instance, not justkeyPrefixkeys.IMPORTANT: the local layer is not supported for sensitive/strong invalidation (trackForInvalidationonly 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 declaredengines.node: ">=18.17"—lru-cache@11requires Node20 || >=22. If you'd rather adopt v11 and drop EOL Node 18, that's a one-line bump to bothlru-cacheandengines.node.Testing
pnpm install --frozen-lockfilepnpm ts:gcache:typecheck— cleanpnpm ts:gcache:test— 73 passed, coverage 97.7% statements / 92.12% branches / 97.65% functions / 97.84% linespnpm ts:gcache:build— esm + cjs + dtsDeferred (non-blocking)
Review items intentionally left for later:
prom-clientloads even withmetrics: false; CIon: push+pull_requestdouble-runs; redundantworkspacesfield in rootpackage.json;gettimer includes config-provider latency; no direct test asserting LRU eviction.🤖 Generated with Claude Code