fix: compare options with an order-insensitive key - #48
Conversation
|
@sidgaikwad is attempting to deploy a commit to the Unlayer Team on Vercel. A member of the Team first needs to authorize it. |
|
@sidgaikwad thank you! Two small things:
Nit: I'll approve the CI run once these land. |
|
Both fixed, and you were right on the nit too. Pushed as 1. Confirmed the stack overflow — reverting the fix makes the new test fail with Now dispatches once and serializes the result directly with
That middle row is the subtle one: the result of 2. Done, both keyed on One thing that fell out of this: Added three tests, each red against a different regression:
They spy on 3. Your nit — you are right, and I have corrected the description.
62 tests, coverage still 100% across the board; lint, typecheck and build clean. Ready for the CI run whenever suits. |
|
Hey @sidgaikwad & @lucasbesen The memo is fine, not blocking. Just a note: It only skips work when The part I'd reconsider is the three tests that assert Anyway, that's just my observation. |
|
@sidgaikwad one issue left: |
|
Right on both counts — fixed in Before, every serializer received Both consequences you named, reproduced:
Fix: thread the key through, matching
Three new tests, all red against exactly the one-token regression ( Each asserts equality with 65 tests, coverage still 100% across the board; lint, typecheck and build clean. One deliberate remaining divergence, for the record: I sort object keys, so the order |
|
@sidgaikwad thank you! Please fix merge conflicts. |
`remountKey` was derived with `JSON.stringify`, which preserves key insertion order. Two deeply equal options objects written with their keys in a different order produced different keys, so the remount effect tore the editor down and recreated it — discarding the canvas, undo/redo history and AI chat. This is easy to hit whenever `options` is assembled conditionally rather than written as one fixed literal. Add `stableKey`, which sorts object keys at every level so the result depends only on content. It otherwise mirrors `JSON.stringify` semantics (undefined/function/symbol omitted from objects, null-filled in arrays), with two deliberate differences: cycles and bigints serialize instead of throwing, since this runs during render. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-ups on the order-insensitive key.
serialize() called itself on the result of toJSON, so a toJSON returning
`this` recursed until the stack overflowed — during render, and before the
cycle guard, which sits after the toJSON branch, could ever run.
Dispatch toJSON exactly once and serialize its result directly, which is
what JSON.stringify does. Properties inside that result still get their own
dispatch, also matching JSON.stringify. All four toJSON tests now assert
parity with JSON.stringify rather than hardcoding expectations alone.
Also memoise remountKey and updatableKey on the options identity, so a
consumer passing a stable object serialises once instead of on every
render. The `options` default is now a module-level constant: a fresh {}
per render would have defeated the memo for everyone who omits the prop.
JSON.stringify calls toJSON(key); stableKey called toJSON() with no
argument, so every serializer saw undefined instead of its key. Two
failure modes, both real:
- A toJSON that reads the argument throws. `key.toUpperCase()` raised
"Cannot read properties of undefined" — during render.
- A toJSON that returns a value derived from the key collapses. One that
returns the slice named by its key produced undefined for every
property, so {theme: slice} and {locale: slice} both serialized to {}
and a genuine option change looked like no change at all.
Thread the key through: '' at the top level, the property name inside an
object, the index as a string inside an array — matching JSON.stringify
at each position.
ec3ef1f to
88aa61e
Compare
|
Rebased onto The only conflict was in
They are independent, so the resolution keeps both. Nothing was dropped from either side — Verified after the rebase rather than assuming the textual merge was sound:
All 8 CI checks are green. The one red mark is Vercel — One note: the comment above |
|
@sidgaikwad one blocking issue remains: memoizing the keys with Please recompute the keys each render, or explicitly make immutable Also, two smaller
|
3 participants
Fixes #31.
Problem
remountKeywas derived withJSON.stringify, which preserves key insertion order:So two deeply equal option objects can produce different keys, and the remount effect destroys and recreates the editor — discarding the canvas, undo/redo history and AI chat:
Easy to hit in real apps, where
optionsis usually assembled conditionally rather than written as one fixed literal.Fix
New
src/stableKey.tssorts object keys at every level, so the key depends only on content. It otherwise mirrorsJSON.stringifysemantics —undefined/function/symbol values omitted from objects, null-filled in arrays, so existing behaviour is unchanged for every input that worked before.Two deliberate divergences, both because this runs during render where a throw would be fatal:
[Circular](JSON.stringifythrows)JSON.stringifythrows)toJSONis honoured. That is not a behaviour change —JSON.stringifyalready honoured it, soa
Dateserialised correctly before. Because this serialiser walks objects itself, the explicittoJSONhandling is what preserves the existing behaviour: without it aDatewould collapse to{}(no own enumerable keys) and two different dates would compare equal.Verification
The new regression test is red without the fix and green with it:
stableKey, 1 new component regression testlint,typecheck,buildall cleanNote
stableKeyis not exported from the package entry point — it is internal.