fix: unstable_cache undefined cache#339
Conversation
|
/bigbonk review |
commit: |
There was a problem hiding this comment.
Clean fix for a real correctness bug. The approach is sound:
JSON.stringify(undefined)returns the JS valueundefined(not a string), soJSON.parse(undefined)throws, and the cache always misses forundefinedresults. The sentinel-based workaround is the right call.- No sentinel collision risk: if a function returns the literal string
"__vinext_unstable_cache_undefined__",JSON.stringifywraps it in quotes, so the serialized form would be'"__vinext_unstable_cache_undefined__"'— never equal to the bare sentinel. - Test is well-structured and directly asserts the bug is fixed (callCount stays at 1 after the second call).
One minor observation below, but nothing blocking.
| function serializeUnstableCacheResult(value: unknown): string { | ||
| return value === undefined | ||
| ? UNSTABLE_CACHE_UNDEFINED_SENTINEL | ||
| : JSON.stringify(value); |
There was a problem hiding this comment.
Nit: JSON.stringify also returns undefined (the JS value) for top-level function and Symbol values. Those are equally un-cacheable with the current code path. Not something that needs fixing in this PR — just noting for awareness that the same class of bug exists for those types. In practice nobody caches functions, so this is fine.
|
Review submitted. Here's a summary: Approved — this is a clean, correct fix for a real caching bug. The problem: The fix: A sentinel string ( I left one minor observation noting that |
Summary
Fix
unstable_cache()so cached functions that return top-levelundefinedactually hit the cache on subsequent calls.Problem
unstable_cache()was serializing results withJSON.stringify(result)and reading them back withJSON.parse(body).For a top-level
undefinedreturn value,JSON.stringify(undefined)returnsundefinedrather than a JSON string. On the next read,JSON.parse(undefined)throws, so the shim treated the entry as invalid and recomputed the function every time.Changes
undefinedresults in theunstable_cache()serialization pathJSON.parseunstable_cache()-wrapped function that returnsundefinedand should only execute once across repeated callsWhy
This is a real correctness/performance bug in the cache shim: callers still receive
undefined, but the cache never produces a true hit for that value.Testing
pnpm test tests/shims.test.ts -t "unstable_cache"