From d694813ea7edaf3f6c27a9bdb665ac1d3b3e015f Mon Sep 17 00:00:00 2001 From: Yash Maheshwari Date: Mon, 7 Sep 2026 20:37:42 +0530 Subject: [PATCH] fix: use a stable option diff to stop spurious remounts and data loss JSON.stringify-keyed option keys were fragile: semantically identical options that differ only in declaration order produced different keys (spurious remounts that discard unsaved edits, or spurious updateOptions), undefined and function values were silently dropped (stale editor config), and circular references crashed at render. Replace the raw JSON diff with a stable canonical serializer (stableKey) that sorts object keys, treats undefined/functions as first-class values, and emits a marker for cycles instead of throwing. Add unit and component tests covering the key-order, dropped-field, and circular-reference cases; full suite stays at 100% coverage. Fixes #66. --- src/ImageEditor.tsx | 7 ++-- src/optionsDiff.ts | 89 ++++++++++++++++++++++++++++++++++++++++ test/index.test.tsx | 56 +++++++++++++++++++++++++ test/optionsDiff.test.ts | 63 ++++++++++++++++++++++++++++ 4 files changed, 212 insertions(+), 3 deletions(-) create mode 100644 src/optionsDiff.ts create mode 100644 test/optionsDiff.test.ts diff --git a/src/ImageEditor.tsx b/src/ImageEditor.tsx index a01c9e8..a9e444d 100644 --- a/src/ImageEditor.tsx +++ b/src/ImageEditor.tsx @@ -7,6 +7,7 @@ import React, { } from 'react'; import { loadScript, resetLoader } from './loadScript'; +import { stableKey } from './optionsDiff'; import { ImageEditorInstance, ImageEditorProps, ImageEditorRef } from './types'; function ImageEditorInner( @@ -66,8 +67,8 @@ function ImageEditorInner( // theme/locale/translations apply via updateOptions; everything else in // options requires a remount. const { theme, locale, translations, ...remountOptions } = options; - const remountKey = JSON.stringify(remountOptions); - const updatableKey = JSON.stringify([theme, locale, translations]); + const remountKey = stableKey(remountOptions); + const updatableKey = stableKey([theme, locale, translations]); useEffect(() => { let cancelled = false; @@ -114,7 +115,7 @@ function ImageEditorInner( instance = created; editorRef.current = created; appliedImageRef.current = mountImage; - appliedUpdatableRef.current = JSON.stringify([ + appliedUpdatableRef.current = stableKey([ mountOptions.theme, mountOptions.locale, mountOptions.translations, diff --git a/src/optionsDiff.ts b/src/optionsDiff.ts new file mode 100644 index 0000000..7b8631d --- /dev/null +++ b/src/optionsDiff.ts @@ -0,0 +1,89 @@ +/** + * Stable, comparison-safe serialization for option-diff detection. + * + * `JSON.stringify` is unsuitable as a React-effect key for the `options` + * prop: + * 1. object keys are emitted in insertion order, so semantically identical + * objects that differ only in property declaration order produce + * different strings (a spurious remount that discards unsaved edits, or + * a spurious updateOptions); + * 2. `undefined` and function values are silently dropped, so real changes + * can be invisible to the diff (stale editor configuration); + * 3. a circular reference throws at render time. + * + * `stableKey` returns a canonical string instead: object keys are sorted, + * `undefined` and functions are first-class values (functions keyed by a + * stable reference id), and circular references are emitted as a marker + * rather than recursing forever. Equal structures always serialize to equal + * strings, regardless of key order, without throwing. + */ + +const UNDEFINED = '\uFFFCundefined'; +const CIRCULAR = '\uFFFCcircular'; +const SYMBOL_PREFIX = '\uFFFCsymbol:'; +const FUNCTION_PREFIX = '\uFFFCfunction:'; +const BIGINT_SUFFIX = 'n'; + +// Functions carry no comparable intrinsic value, so key them by reference: +// the same function instance is stable across renders, while a freshly +// created closure is a genuine change the diff must not miss. +const functionIds = new WeakMap(); +let nextFunctionId = 0; + +const functionId = (fn: Function): number => { + const existing = functionIds.get(fn); + if (existing !== undefined) return existing; + const id = nextFunctionId++; + functionIds.set(fn, id); + return id; +}; + +const serialize = (value: unknown, path: Set): string => { + switch (typeof value) { + case 'string': + return JSON.stringify(value); + case 'number': + case 'boolean': + return String(value); + case 'bigint': + return `${value}${BIGINT_SUFFIX}`; + case 'undefined': + return UNDEFINED; + case 'symbol': + return `${SYMBOL_PREFIX}${String(value.description ?? '')}`; + case 'function': + return `${FUNCTION_PREFIX}${functionId(value)}`; + case 'object': + if (value === null) return 'null'; + // A node already on the active path is a cycle: emit a marker and stop + // recursing so a circular `options` object can no longer crash render. + if (path.has(value)) return CIRCULAR; + path.add(value); + let result: string; + if (Array.isArray(value)) { + result = `[${value.map((item) => serialize(item, path)).join(',')}]`; + } else { + const entries = Object.entries(value) + .sort(([left], [right]) => left.localeCompare(right)) + .map( + ([key, entry]) => `${JSON.stringify(key)}:${serialize(entry, path)}` + ) + .join(','); + result = `{${entries}}`; + } + path.delete(value); + return result; + /* v8 ignore next -- every typeof is handled by a case above */ + default: + return 'null'; + } +}; + +/** + * Canonical serializer for structural option diffing. See the module doc for + * the guarantees it provides over `JSON.stringify`. + */ +export const stableKey = (value: unknown): string => { + const path = new Set(); + return serialize(value, path); +}; diff --git a/test/index.test.tsx b/test/index.test.tsx index 5c08aa2..541fb15 100644 --- a/test/index.test.tsx +++ b/test/index.test.tsx @@ -321,6 +321,62 @@ it('remounts the editor when a non-updatable option changes', async () => { expect(mountOptionsOf(1).projectId).toBe(2); }); +it('does not remount or update options when only the key order changes', async () => { + const { rerender } = render( + + ); + await flush(); + + rerender( + + ); + await flush(); + + // Key-order-only differences are semantically identical and must neither + // destroy the editor nor push live options updates. + expect(mockInstance.destroy).not.toHaveBeenCalled(); + expect(createEditor).toHaveBeenCalledTimes(1); + expect(mockInstance.updateOptions).not.toHaveBeenCalled(); +}); + +it('detects a change in a previously-undefined option value', async () => { + const { rerender } = render( + + ); + await flush(); + + rerender( + + ); + await flush(); + + // JSON.stringify would have dropped the undefined name and treated both + // options as identical; the stable diff must see the real change. + expect(mockInstance.destroy).toHaveBeenCalledTimes(1); + expect(createEditor).toHaveBeenCalledTimes(2); + expect(mountOptionsOf(1).user?.name).toBe('Adeel'); +}); + +it('renders without crashing when options contain a circular reference', async () => { + const options: ImageEditorOptions = { projectId: 1 } as ImageEditorOptions; + const self: Record = {}; + self.user = self; + options.user = self as ImageEditorOptions['user']; + + render(); + await flush(); + + // A circular options object must not throw at render; the editor mounts once. + expect(createEditor).toHaveBeenCalledTimes(1); + expect(mockInstance.destroy).not.toHaveBeenCalled(); +}); + it('always invokes the latest callbacks', async () => { const firstOnSave = vi.fn(); const secondOnSave = vi.fn(); diff --git a/test/optionsDiff.test.ts b/test/optionsDiff.test.ts new file mode 100644 index 0000000..7416f81 --- /dev/null +++ b/test/optionsDiff.test.ts @@ -0,0 +1,63 @@ +import { stableKey } from '../src/optionsDiff'; + +describe('stableKey', () => { + it('produces equal keys for objects whose properties are in different order', () => { + expect( + stableKey({ a: { x: 1, y: { p: 2, q: 3 } }, b: 4, c: [1, 2, 3] }) + ).toBe(stableKey({ c: [1, 2, 3], b: 4, a: { y: { q: 3, p: 2 }, x: 1 } })); + }); + + it('distinguishes different values', () => { + expect(stableKey({ a: 1 })).not.toBe(stableKey({ a: 2 })); + expect(stableKey('x')).not.toBe(stableKey('y')); + expect(stableKey([1])).not.toBe(stableKey([1, 2])); + expect(stableKey(null)).not.toBe(stableKey({})); + }); + + it('treats undefined as a first-class value so dropped fields stay visible', () => { + // JSON.stringify omits undefined; here it must be distinguishable from + // both an absent key and a present value. + expect(stableKey({ a: undefined })).not.toBe(stableKey({})); + expect(stableKey({ a: undefined })).not.toBe(stableKey({ a: 1 })); + expect(stableKey({ a: undefined })).toBe(stableKey({ a: undefined })); + expect(stableKey([undefined])).not.toBe(stableKey([])); + }); + + it('keys functions by reference: the same instance is stable, a new one differs', () => { + const callback = () => {}; + expect(stableKey({ cb: callback })).toBe(stableKey({ cb: callback })); + expect(stableKey({ cb: () => {} })).not.toBe(stableKey({ cb: () => {} })); + }); + + it('does not throw on circular references and emits a stable marker', () => { + const first: { name: string; self?: unknown } = { name: 'a' }; + first.self = first; + const second: { name: string; self?: unknown } = { name: 'a' }; + second.self = second; + + expect(() => stableKey(first)).not.toThrow(); + expect(stableKey(first)).toBe(stableKey(second)); + expect(stableKey(first)).not.toBe(stableKey({ name: 'b', self: first })); + + // A node already on the active path inside an array is handled too. + const cycles: unknown[] = [1, 2]; + cycles.push(cycles); + expect(() => stableKey(cycles)).not.toThrow(); + }); + + it('serializes every primitive value type without throwing', () => { + expect(() => + stableKey({ + str: 'text', + num: 42, + bool: false, + big: BigInt(10), + nil: null, + sym: Symbol('sym'), + bareSym: Symbol(), + undef: undefined, + fn: () => {}, + }) + ).not.toThrow(); + }); +});