Skip to content

fix: keep -0 and +0 distinct through serialize/deserialize#25

Open
greymoth-jp wants to merge 2 commits into
ungap:mainfrom
greymoth-jp:fix-negative-zero-serialize
Open

fix: keep -0 and +0 distinct through serialize/deserialize#25
greymoth-jp wants to merge 2 commits into
ungap:mainfrom
greymoth-jp:fix-negative-zero-serialize

Conversation

@greymoth-jp

Copy link
Copy Markdown

serialize memoizes every value it has seen in a Map so shared references and cycles round-trip as one record. Map keys use SameValueZero, which treats -0 and +0 as the same key, so when both appear in one value the second one reuses the first one's record. After deserialize they come back with a single sign:

const {serialize, deserialize} = require('@ungap/structured-clone');

deserialize(serialize([-0, 0]));        // [-0, -0]   (expected [-0, 0])
deserialize(serialize([0, -0]));        // [0, 0]     (expected [0, -0])
deserialize(serialize({neg: -0, pos: 0})); // {neg: -0, pos: -0}

Native structuredClone and this module's own default clone (which copies values directly instead of going through the memo) both keep the two zeros distinct, so serialize/deserialize is the only path that loses the difference.

The fix skips the memo for negative zero only, so -0 can never share a record with +0. Every other value memoizes exactly as before, so reference sharing, cycles and the existing serialized output are unchanged. A Set still collapses -0/+0 into one element, matching native, because that happens in the Set itself and not in the memo.

Added a round-trip test alongside the Invalid Date one. The suite passes; reverting the serialize.js change makes the new test fail.

The serialize memo is a Map, whose keys use SameValueZero, so -0 and +0
share a record when both appear in one value and collapse to a single
sign after deserialize. Skip the memo for negative zero so it never
shares a record with +0; every other value memoizes as before, so
reference sharing, cycles and the existing serialized output are
unchanged.
@WebReflection

Copy link
Copy Markdown
Member

I would do that only if TYPE is number and it's falsy otherwise this is a huge performance loss

Guard the negative-zero check so Object.is runs only when the value is a
zero-valued number; non-numbers and nonzero numbers short-circuit before
it, so the per-value memo cost is unchanged from before this fix.
@greymoth-jp

Copy link
Copy Markdown
Author

Gated the -0 check so Object.is only runs on zero-valued numbers now, everything else short-circuits before it, so the per-value cost is gone. Existing tests plus the -0/+0 round-trip pass. Thanks for the review.

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.

2 participants