add try/catch statement around serialize method in writeStagedState()#55
add try/catch statement around serialize method in writeStagedState()#55mdemichele wants to merge 3 commits into
Conversation
|
Will need a better PR description and a unit test or two for this before I merge in. Still WIP. |
…erializing The try/catch guard was correctly capturing the serialized output but then calling serialize(stagedState) again in storage.setItem, leaving the second call unguarded and defeating the purpose of the fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verifies that a synchronous RangeError from serialize() in writeStagedState is routed to writeFailHandler instead of propagating as an unhandled exception, and that storage.setItem is never called when serialization fails. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mdemichele
left a comment
There was a problem hiding this comment.
PR #55 Review: writeStagedState Serialization Error Handling
Overview
Fixes issue #54 by wrapping the serialize(stagedState) call in writeStagedState() in a try/catch, routing synchronous serialization failures (e.g., RangeError: String length exceeds limit on Hermes) to onWriteFail instead of letting them crash the app. Includes a unit test validating the fix.
Code Correctness ✅
- The fix is structurally correct and directly mirrors the existing guard already present in
processNextKeyfor per-key serialization — good consistency. - The
returnafteronWriteFail(err)correctly short-circuits beforestorage.setItemis called. - The
serializedvariable is correctly used instorage.setItemon line 108, avoiding the redundant double-serialize call that was present in the initial commit on this branch.
Potential Issues
writePromise is not reset on failure
When serialization fails, writePromise is left holding whatever it was previously (could be a stale resolved promise from an earlier write, could be null). This means a caller awaiting flush() after a failed write gets back a stale promise rather than one that reflects the failure. This is probably fine for now since writeFailHandler is the intended signal, but worth noting.
let serialized is untyped
let serialized
try {
serialized = serialize(stagedState)
} catch ...TypeScript infers this as any. A minor style point — const serialize is typed as (x: any) => any so this is consistent with the rest of the file, but an explicit annotation (let serialized: any) would be clearer about the intent.
Test Coverage
What the test does well:
- Validates all three key behaviors: no throw,
writeFailHandlercalled,storage.setItemnever called. - Uses
t.is(writeFailHandler.firstCall.args[0], rangeError)to assert the exact error object is passed through — not just that some error was passed. - Uses
flush()rather thanclock.tick(), making the test synchronous and easier to reason about.
One gap worth considering:
The test's serialize throws on every call — including the per-key path in processNextKey (which is already guarded). This means stagedState ends up empty {} before writeStagedState runs. The actual real-world scenario from the issue is subtly different: per-key serialization succeeds, but combining them into a whole-state JSON string exceeds the engine's limit. A complementary test could look like:
// Per-key succeeds, whole-state serialize fails (faithful to the Hermes crash)
let callCount = 0
const serialize = (data: any) => {
if (callCount++ > 0) throw new RangeError('String length exceeds limit')
return JSON.stringify(data)
}This isn't a blocker, but it would make the test suite more precisely match the reported failure mode.
PR Description
"Addresses Issue #54" is minimal. Would be worth adding a brief summary of what was changed (the try/catch guard) and why the serialized variable fix was needed (the initial commit still called serialize twice, leaving the guard ineffective).
Summary
| Correctness | ✅ Fix is sound and addresses the root cause |
| Style | ✅ Consistent with existing patterns in the file |
| Tests | ✅ Covers the critical path; one gap noted (always-throwing vs. whole-state-only-throwing) |
| Risk | Low — the change is minimal and scoped entirely to the error path |
The fix is good to merge. The gap in test coverage is a nice-to-have, not a blocker.
Addresses Issue #54.