From ea7faba9baee66e0742112a7ef5a6948c4f7e010 Mon Sep 17 00:00:00 2001 From: sidgaikwad Date: Fri, 4 Sep 2026 11:51:14 +0530 Subject: [PATCH 1/5] fix: compare options with an order-insensitive key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- src/ImageEditor.tsx | 10 +++++-- src/stableKey.ts | 54 ++++++++++++++++++++++++++++++++++ test/index.test.tsx | 17 +++++++++++ test/stableKey.test.ts | 67 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 src/stableKey.ts create mode 100644 test/stableKey.test.ts diff --git a/src/ImageEditor.tsx b/src/ImageEditor.tsx index acae488..b8d7724 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 './stableKey'; import { ImageEditorInstance, ImageEditorProps, ImageEditorRef } from './types'; function ImageEditorInner( @@ -74,8 +75,11 @@ 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]); + // stableKey, not JSON.stringify: the latter is key-order sensitive, so a + // deeply equal options object written with its keys in a different order + // would remount the editor and discard the user's unsaved work. + const remountKey = stableKey(remountOptions); + const updatableKey = stableKey([theme, locale, translations]); useEffect(() => { let cancelled = false; @@ -122,7 +126,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/stableKey.ts b/src/stableKey.ts new file mode 100644 index 0000000..bdf7432 --- /dev/null +++ b/src/stableKey.ts @@ -0,0 +1,54 @@ +/** + * Deterministic serialization used to detect option changes across renders. + * + * `JSON.stringify` preserves key insertion order, so two deeply equal option + * objects can serialize differently and trigger a needless remount. This + * sorts object keys at every level, so the result depends only on content. + * + * Semantics otherwise mirror `JSON.stringify` (undefined/function/symbol + * values are omitted from objects and become `null` in arrays), with two + * deliberate differences: cycles serialize as `[Circular]` and bigints + * serialize as strings, because `JSON.stringify` throws on both and this + * runs during render. + */ +const serialize = (value: unknown, seen: Set): string | undefined => { + if (typeof value === 'bigint') return `"${value}"`; + + // Covers primitives, plus the omitted-value cases (undefined, function, + // symbol) where JSON.stringify itself returns undefined. + if (value === null || typeof value !== 'object') return JSON.stringify(value); + + const object = value as { toJSON?: () => unknown }; + if (typeof object.toJSON === 'function') { + return serialize(object.toJSON(), seen); + } + + if (seen.has(value)) return '"[Circular]"'; + seen.add(value); + + let result: string; + if (Array.isArray(value)) { + result = `[${value.map((item) => serialize(item, seen) ?? 'null').join(',')}]`; + } else { + const entries: string[] = []; + for (const key of Object.keys(value).sort()) { + const serialized = serialize( + (value as Record)[key], + seen + ); + if (serialized !== undefined) { + entries.push(`${JSON.stringify(key)}:${serialized}`); + } + } + result = `{${entries.join(',')}}`; + } + + seen.delete(value); + return result; +}; + +/** + * A key that is equal for deeply equal values, regardless of key order. + */ +export const stableKey = (value: unknown): string => + serialize(value, new Set()) ?? 'undefined'; diff --git a/test/index.test.tsx b/test/index.test.tsx index 909c577..e708d43 100644 --- a/test/index.test.tsx +++ b/test/index.test.tsx @@ -188,6 +188,23 @@ it('does not remount when the tools config is deep-equal but not reference-equal expect(createEditor).toHaveBeenCalledTimes(1); }); +it('does not remount when the same options are written in a different key order', async () => { + const { rerender } = render( + + ); + await flush(); + + // Same configuration, keys in the other order — the common case when + // options are assembled conditionally rather than as one fixed literal. + rerender( + + ); + await flush(); + + expect(mockInstance.destroy).not.toHaveBeenCalled(); + expect(createEditor).toHaveBeenCalledTimes(1); +}); + it('exposes the editor instance through the ref and calls onLoad', async () => { const ref = React.createRef(); const onLoad = vi.fn(); diff --git a/test/stableKey.test.ts b/test/stableKey.test.ts new file mode 100644 index 0000000..200cde4 --- /dev/null +++ b/test/stableKey.test.ts @@ -0,0 +1,67 @@ +import { stableKey } from '../src/stableKey'; + +it('is insensitive to object key order at every level', () => { + const a = { projectId: 1, features: { imageEditor: { dock: 'left' } } }; + const b = { features: { imageEditor: { dock: 'left' } }, projectId: 1 }; + + expect(stableKey(a)).toBe(stableKey(b)); + // The bug this exists to prevent: JSON.stringify disagrees here. + expect(JSON.stringify(a)).not.toBe(JSON.stringify(b)); +}); + +it('still distinguishes different values', () => { + expect(stableKey({ a: 1 })).not.toBe(stableKey({ a: 2 })); + expect(stableKey({ a: 1 })).not.toBe(stableKey({ b: 1 })); + expect(stableKey({ a: 1 })).not.toBe(stableKey({ a: '1' })); +}); + +it('preserves array order', () => { + expect(stableKey([1, 2])).not.toBe(stableKey([2, 1])); + expect(stableKey([{ b: 1, a: 2 }])).toBe(stableKey([{ a: 2, b: 1 }])); +}); + +it('mirrors JSON.stringify for omitted values', () => { + // Omitted from objects... + expect(stableKey({ a: undefined, b: 1 })).toBe(stableKey({ b: 1 })); + expect(stableKey({ a: () => {}, b: 1 })).toBe(stableKey({ b: 1 })); + expect(stableKey({ a: Symbol('s'), b: 1 })).toBe(stableKey({ b: 1 })); + // ...but null-filled in arrays, so positions do not shift. + expect(stableKey([undefined, 1])).toBe('[null,1]'); +}); + +it('handles primitives, null and non-finite numbers', () => { + expect(stableKey(undefined)).toBe('undefined'); + expect(stableKey(null)).toBe('null'); + expect(stableKey('x')).toBe('"x"'); + expect(stableKey(1)).toBe('1'); + expect(stableKey(true)).toBe('true'); + expect(stableKey(NaN)).toBe('null'); +}); + +it('serializes bigints instead of throwing', () => { + // BigInt(), not a 1n literal: tsconfig targets es2019. + expect(() => JSON.stringify({ a: BigInt(1) })).toThrow(); + expect(stableKey({ a: BigInt(1) })).toBe(stableKey({ a: BigInt(1) })); + expect(stableKey({ a: BigInt(1) })).not.toBe(stableKey({ a: BigInt(2) })); +}); + +it('serializes cycles instead of throwing', () => { + const cyclic: Record = { a: 1 }; + cyclic.self = cyclic; + + expect(() => JSON.stringify(cyclic)).toThrow(); + expect(stableKey(cyclic)).toBe('{"a":1,"self":"[Circular]"}'); +}); + +it('does not treat a repeated sibling as a cycle', () => { + const shared = { a: 1 }; + + expect(stableKey({ x: shared, y: shared })).toBe( + stableKey({ x: { a: 1 }, y: { a: 1 } }) + ); +}); + +it('honours toJSON, so equal Dates compare equal', () => { + expect(stableKey(new Date(0))).toBe(stableKey(new Date(0))); + expect(stableKey(new Date(0))).not.toBe(stableKey(new Date(1))); +}); From d97de3983cf2d8705d90a3d8dfbab82424b48366 Mon Sep 17 00:00:00 2001 From: sidgaikwad Date: Fri, 4 Sep 2026 21:12:27 +0530 Subject: [PATCH 2/5] fix: dispatch toJSON once, and memoise the option keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/ImageEditor.tsx | 30 ++++++++++++++++++++---- src/stableKey.ts | 15 +++++++++--- test/index.test.tsx | 53 ++++++++++++++++++++++++++++++++++++++++++ test/stableKey.test.ts | 31 ++++++++++++++++++++++-- 4 files changed, 120 insertions(+), 9 deletions(-) diff --git a/src/ImageEditor.tsx b/src/ImageEditor.tsx index b8d7724..926175b 100644 --- a/src/ImageEditor.tsx +++ b/src/ImageEditor.tsx @@ -2,13 +2,23 @@ import React, { useEffect, useId, useImperativeHandle, + useMemo, useRef, useState, } from 'react'; import { loadScript, resetLoader } from './loadScript'; import { stableKey } from './stableKey'; -import { ImageEditorInstance, ImageEditorProps, ImageEditorRef } from './types'; +import { + ImageEditorInstance, + ImageEditorOptions, + ImageEditorProps, + ImageEditorRef, +} from './types'; + +// A stable default, so omitting the `options` prop does not hand the memos +// below a fresh object identity on every render. +const NO_OPTIONS: ImageEditorOptions = {}; function ImageEditorInner( props: ImageEditorProps, @@ -16,7 +26,7 @@ function ImageEditorInner( ) { const { image, - options = {}, + options = NO_OPTIONS, scriptUrl, minHeight = 500, style = {}, @@ -78,8 +88,20 @@ function ImageEditorInner( // stableKey, not JSON.stringify: the latter is key-order sensitive, so a // deeply equal options object written with its keys in a different order // would remount the editor and discard the user's unsaved work. - const remountKey = stableKey(remountOptions); - const updatableKey = stableKey([theme, locale, translations]); + // + // Memoised on the options identity: a consumer passing a stable object + // (or omitting the prop) serialises once rather than on every render. + // Everything both keys read comes from `options`, so it is the only dep. + const remountKey = useMemo( + () => stableKey(remountOptions), + // eslint-disable-next-line react-hooks/exhaustive-deps + [options] + ); + const updatableKey = useMemo( + () => stableKey([theme, locale, translations]), + // eslint-disable-next-line react-hooks/exhaustive-deps + [options] + ); useEffect(() => { let cancelled = false; diff --git a/src/stableKey.ts b/src/stableKey.ts index bdf7432..99ca408 100644 --- a/src/stableKey.ts +++ b/src/stableKey.ts @@ -11,7 +11,11 @@ * serialize as strings, because `JSON.stringify` throws on both and this * runs during render. */ -const serialize = (value: unknown, seen: Set): string | undefined => { +const serialize = ( + value: unknown, + seen: Set, + honourToJSON = true +): string | undefined => { if (typeof value === 'bigint') return `"${value}"`; // Covers primitives, plus the omitted-value cases (undefined, function, @@ -19,8 +23,13 @@ const serialize = (value: unknown, seen: Set): string | undefined => { if (value === null || typeof value !== 'object') return JSON.stringify(value); const object = value as { toJSON?: () => unknown }; - if (typeof object.toJSON === 'function') { - return serialize(object.toJSON(), seen); + if (honourToJSON && typeof object.toJSON === 'function') { + // Dispatch toJSON exactly once and serialize its result directly, as + // JSON.stringify does. Re-dispatching would let a toJSON that returns + // `this` recurse until the stack overflows — during render, before the + // cycle guard below is ever reached. Properties *inside* the result + // still get their own dispatch, which is also what JSON.stringify does. + return serialize(object.toJSON(), seen, false); } if (seen.has(value)) return '"[Circular]"'; diff --git a/test/index.test.tsx b/test/index.test.tsx index e708d43..c0ff5e9 100644 --- a/test/index.test.tsx +++ b/test/index.test.tsx @@ -9,6 +9,7 @@ import ImageEditor, { MountOptions, } from '../src'; import { loadScript, resetLoader } from '../src/loadScript'; +import { stableKey } from '../src/stableKey'; // Resolve the embed script immediately instead of hitting the network. vi.mock('../src/loadScript', () => ({ @@ -16,6 +17,13 @@ vi.mock('../src/loadScript', () => ({ resetLoader: vi.fn(), })); +// Spy only — the real implementation still runs, so behaviour is unchanged. +// Lets a test assert that the option keys are actually memoised. +vi.mock('../src/stableKey', async (importOriginal) => { + const actual = await importOriginal(); + return { stableKey: vi.fn(actual.stableKey) }; +}); + interface Deferred { promise: Promise; resolve: (value: T) => void; @@ -61,6 +69,7 @@ beforeEach(() => { vi.mocked(loadScript).mockImplementation(() => Promise.resolve()); vi.mocked(resetLoader).mockClear(); + vi.mocked(stableKey).mockClear(); mockInstance = makeInstance(); createEditor = vi.fn(async () => mockInstance); window.ImageEditor = { @@ -205,6 +214,50 @@ it('does not remount when the same options are written in a different key order' expect(createEditor).toHaveBeenCalledTimes(1); }); +it('does not re-serialize options when the object identity is stable', async () => { + const options: ImageEditorOptions = { projectId: 1234, offline: false }; + const { rerender } = render(); + await flush(); + + const afterMount = vi.mocked(stableKey).mock.calls.length; + expect(afterMount).toBeGreaterThan(0); + + // Same object, three more renders: the memos should absorb all of them. + rerender(); + rerender(); + rerender(); + await flush(); + + expect(vi.mocked(stableKey).mock.calls.length).toBe(afterMount); +}); + +it('re-serializes when a fresh options object arrives', async () => { + const { rerender } = render( + + ); + await flush(); + const afterMount = vi.mocked(stableKey).mock.calls.length; + + rerender(); + + expect(vi.mocked(stableKey).mock.calls.length).toBeGreaterThan(afterMount); + // ...and still resolves to the same key, so no remount. + expect(createEditor).toHaveBeenCalledTimes(1); +}); + +it('does not serialize a fresh empty default on every render', async () => { + const { rerender } = render(); + await flush(); + const afterMount = vi.mocked(stableKey).mock.calls.length; + + rerender(); + rerender(); + await flush(); + + // Omitting `options` used to hand the memo a new {} each render. + expect(vi.mocked(stableKey).mock.calls.length).toBe(afterMount); +}); + it('exposes the editor instance through the ref and calls onLoad', async () => { const ref = React.createRef(); const onLoad = vi.fn(); diff --git a/test/stableKey.test.ts b/test/stableKey.test.ts index 200cde4..e464d34 100644 --- a/test/stableKey.test.ts +++ b/test/stableKey.test.ts @@ -61,7 +61,34 @@ it('does not treat a repeated sibling as a cycle', () => { ); }); -it('honours toJSON, so equal Dates compare equal', () => { - expect(stableKey(new Date(0))).toBe(stableKey(new Date(0))); +it('honours toJSON, matching JSON.stringify', () => { + expect(stableKey(new Date(0))).toBe(JSON.stringify(new Date(0))); expect(stableKey(new Date(0))).not.toBe(stableKey(new Date(1))); }); + +it('does not recurse when toJSON returns this', () => { + // Dispatching toJSON on its own result would recurse forever here, and + // the cycle guard never runs because the toJSON branch precedes it. + const selfish = { a: 1, toJSON: () => selfish }; + + expect(() => stableKey(selfish)).not.toThrow(); + expect(stableKey(selfish)).toBe(JSON.stringify(selfish)); + expect(stableKey(selfish)).toBe('{"a":1}'); +}); + +it('does not re-dispatch toJSON on its own result', () => { + const chained = { toJSON: () => ({ toJSON: () => 'SECOND' }) }; + + // JSON.stringify serialises the result's own keys rather than calling its + // toJSON, so the function-valued key is dropped and this collapses to {}. + expect(stableKey(chained)).toBe(JSON.stringify(chained)); + expect(stableKey(chained)).toBe('{}'); +}); + +it('still dispatches toJSON for properties inside a toJSON result', () => { + const inner = { toJSON: () => 'INNER' }; + const outer = { toJSON: () => ({ nested: inner, plain: 1 }) }; + + expect(stableKey(outer)).toBe(JSON.stringify(outer)); + expect(stableKey(outer)).toBe('{"nested":"INNER","plain":1}'); +}); From 88aa61e595c9574642a332780e7b4c7d6ee7c0d9 Mon Sep 17 00:00:00 2001 From: sidgaikwad Date: Tue, 8 Sep 2026 23:59:54 +0530 Subject: [PATCH 3/5] fix: forward the property key to toJSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/stableKey.ts | 23 +++++++++++++++-------- test/stableKey.test.ts | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/src/stableKey.ts b/src/stableKey.ts index 99ca408..f28acac 100644 --- a/src/stableKey.ts +++ b/src/stableKey.ts @@ -14,6 +14,10 @@ const serialize = ( value: unknown, seen: Set, + // The property name this value sits under, forwarded to toJSON exactly as + // JSON.stringify does: '' at the top level, the property name inside an + // object, the index as a string inside an array. + key: string, honourToJSON = true ): string | undefined => { if (typeof value === 'bigint') return `"${value}"`; @@ -22,14 +26,14 @@ const serialize = ( // symbol) where JSON.stringify itself returns undefined. if (value === null || typeof value !== 'object') return JSON.stringify(value); - const object = value as { toJSON?: () => unknown }; + const object = value as { toJSON?: (key: string) => unknown }; if (honourToJSON && typeof object.toJSON === 'function') { // Dispatch toJSON exactly once and serialize its result directly, as // JSON.stringify does. Re-dispatching would let a toJSON that returns // `this` recurse until the stack overflows — during render, before the // cycle guard below is ever reached. Properties *inside* the result // still get their own dispatch, which is also what JSON.stringify does. - return serialize(object.toJSON(), seen, false); + return serialize(object.toJSON(key), seen, key, false); } if (seen.has(value)) return '"[Circular]"'; @@ -37,16 +41,19 @@ const serialize = ( let result: string; if (Array.isArray(value)) { - result = `[${value.map((item) => serialize(item, seen) ?? 'null').join(',')}]`; + result = `[${value + .map((item, index) => serialize(item, seen, String(index)) ?? 'null') + .join(',')}]`; } else { const entries: string[] = []; - for (const key of Object.keys(value).sort()) { + for (const name of Object.keys(value).sort()) { const serialized = serialize( - (value as Record)[key], - seen + (value as Record)[name], + seen, + name ); if (serialized !== undefined) { - entries.push(`${JSON.stringify(key)}:${serialized}`); + entries.push(`${JSON.stringify(name)}:${serialized}`); } } result = `{${entries.join(',')}}`; @@ -60,4 +67,4 @@ const serialize = ( * A key that is equal for deeply equal values, regardless of key order. */ export const stableKey = (value: unknown): string => - serialize(value, new Set()) ?? 'undefined'; + serialize(value, new Set(), '') ?? 'undefined'; diff --git a/test/stableKey.test.ts b/test/stableKey.test.ts index e464d34..e5e712d 100644 --- a/test/stableKey.test.ts +++ b/test/stableKey.test.ts @@ -85,6 +85,44 @@ it('does not re-dispatch toJSON on its own result', () => { expect(stableKey(chained)).toBe('{}'); }); +it('passes the property key to toJSON, as JSON.stringify does', () => { + const rec = { toJSON: (key: string) => `key=${JSON.stringify(key)}` }; + + // '' at the top level, the property name in an object, the index as a + // string in an array. + expect(stableKey(rec)).toBe(JSON.stringify(rec)); + expect(stableKey({ a: rec, bb: rec })).toBe( + JSON.stringify({ a: rec, bb: rec }) + ); + expect(stableKey([rec, rec])).toBe(JSON.stringify([rec, rec])); + expect(stableKey({ outer: { inner: rec } })).toBe( + JSON.stringify({ outer: { inner: rec } }) + ); +}); + +it('does not crash a toJSON that reads its key argument', () => { + // Calling toJSON() with no argument handed these `undefined`, so anything + // touching the key threw — during render. + const strict = { toJSON: (key: string) => key.toUpperCase() }; + + expect(() => stableKey({ ab: strict })).not.toThrow(); + expect(stableKey({ ab: strict })).toBe(JSON.stringify({ ab: strict })); +}); + +it('does not collapse distinct options whose toJSON reads the key', () => { + // Returns the slice named by the key; with an undefined key it returned + // undefined for everything, so both of these serialized to `{}` and a + // real option change looked like no change at all. + const slice = { + toJSON: (key: string) => + ({ theme: 'dark', locale: 'fr' })[key as 'theme' | 'locale'], + }; + + expect(stableKey({ theme: slice })).toBe(JSON.stringify({ theme: slice })); + expect(stableKey({ locale: slice })).toBe(JSON.stringify({ locale: slice })); + expect(stableKey({ theme: slice })).not.toBe(stableKey({ locale: slice })); +}); + it('still dispatches toJSON for properties inside a toJSON result', () => { const inner = { toJSON: () => 'INNER' }; const outer = { toJSON: () => ({ nested: inner, plain: 1 }) }; From e40f028cdb10423e7af3cab56c2bb79fb7d7a77f Mon Sep 17 00:00:00 2001 From: sidgaikwad Date: Sat, 12 Sep 2026 14:08:39 +0530 Subject: [PATCH 4/5] fix: recompute the option keys every render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Memoising remountKey/updatableKey on the `options` identity meant an options object mutated in place was never re-serialised, so the keys went stale and projectId, theme and feature settings silently kept their old values. The released version called JSON.stringify on every render and did detect this, making the memo a backward-compatibility regression. Drop both memos and serialise every render, as the release did. Callers may hold one long-lived options object and mutate it; serialising a small object is far cheaper than the remount a missed change costs. Removes the two tests that asserted the memo absorbed repeat renders — they encoded exactly the behaviour being reverted. Adds coverage for in-place mutation at both tiers: a remount-tier key (projectId) and an updateOptions-tier key (theme). --- src/ImageEditor.tsx | 35 +++++++++--------------------- test/index.test.tsx | 53 +++++++++++++++++++++++++-------------------- 2 files changed, 39 insertions(+), 49 deletions(-) diff --git a/src/ImageEditor.tsx b/src/ImageEditor.tsx index 926175b..c6c994a 100644 --- a/src/ImageEditor.tsx +++ b/src/ImageEditor.tsx @@ -2,23 +2,13 @@ import React, { useEffect, useId, useImperativeHandle, - useMemo, useRef, useState, } from 'react'; import { loadScript, resetLoader } from './loadScript'; import { stableKey } from './stableKey'; -import { - ImageEditorInstance, - ImageEditorOptions, - ImageEditorProps, - ImageEditorRef, -} from './types'; - -// A stable default, so omitting the `options` prop does not hand the memos -// below a fresh object identity on every render. -const NO_OPTIONS: ImageEditorOptions = {}; +import { ImageEditorInstance, ImageEditorProps, ImageEditorRef } from './types'; function ImageEditorInner( props: ImageEditorProps, @@ -26,7 +16,7 @@ function ImageEditorInner( ) { const { image, - options = NO_OPTIONS, + options = {}, scriptUrl, minHeight = 500, style = {}, @@ -89,19 +79,14 @@ function ImageEditorInner( // deeply equal options object written with its keys in a different order // would remount the editor and discard the user's unsaved work. // - // Memoised on the options identity: a consumer passing a stable object - // (or omitting the prop) serialises once rather than on every render. - // Everything both keys read comes from `options`, so it is the only dep. - const remountKey = useMemo( - () => stableKey(remountOptions), - // eslint-disable-next-line react-hooks/exhaustive-deps - [options] - ); - const updatableKey = useMemo( - () => stableKey([theme, locale, translations]), - // eslint-disable-next-line react-hooks/exhaustive-deps - [options] - ); + // Deliberately recomputed every render, NOT memoised on the options + // identity. Callers are free to mutate one long-lived options object in + // place, and a memo keyed on that object would never see it change, + // leaving projectId/theme/feature settings silently stale. Serializing a + // small options object is far cheaper than the remount a missed change + // costs, and it is what the released JSON.stringify did. + const remountKey = stableKey(remountOptions); + const updatableKey = stableKey([theme, locale, translations]); useEffect(() => { let cancelled = false; diff --git a/test/index.test.tsx b/test/index.test.tsx index c0ff5e9..706b7ed 100644 --- a/test/index.test.tsx +++ b/test/index.test.tsx @@ -214,23 +214,6 @@ it('does not remount when the same options are written in a different key order' expect(createEditor).toHaveBeenCalledTimes(1); }); -it('does not re-serialize options when the object identity is stable', async () => { - const options: ImageEditorOptions = { projectId: 1234, offline: false }; - const { rerender } = render(); - await flush(); - - const afterMount = vi.mocked(stableKey).mock.calls.length; - expect(afterMount).toBeGreaterThan(0); - - // Same object, three more renders: the memos should absorb all of them. - rerender(); - rerender(); - rerender(); - await flush(); - - expect(vi.mocked(stableKey).mock.calls.length).toBe(afterMount); -}); - it('re-serializes when a fresh options object arrives', async () => { const { rerender } = render( @@ -245,17 +228,39 @@ it('re-serializes when a fresh options object arrives', async () => { expect(createEditor).toHaveBeenCalledTimes(1); }); -it('does not serialize a fresh empty default on every render', async () => { - const { rerender } = render(); +it('detects an in-place mutation of a long-lived options object', async () => { + // A consumer holding one config object and mutating it is supported by the + // released version, which re-serialised on every render. Memoising the key + // on the options identity would silently miss this and leave the editor + // configured with the old projectId. + const options: ImageEditorOptions = { projectId: 1 }; + const { rerender } = render(); await flush(); - const afterMount = vi.mocked(stableKey).mock.calls.length; - rerender(); - rerender(); + expect(createEditor).toHaveBeenCalledTimes(1); + + options.projectId = 2; + rerender(); + await flush(); + + expect(mockInstance.destroy).toHaveBeenCalledTimes(1); + expect(createEditor).toHaveBeenCalledTimes(2); + expect(mountOptionsOf(1).projectId).toBe(2); +}); + +it('detects an in-place theme mutation without remounting', async () => { + const options: ImageEditorOptions = { theme: 'light' }; + const { rerender } = render(); + await flush(); + + options.theme = 'dark'; + rerender(); await flush(); - // Omitting `options` used to hand the memo a new {} each render. - expect(vi.mocked(stableKey).mock.calls.length).toBe(afterMount); + expect(mockInstance.updateOptions).toHaveBeenCalledWith( + expect.objectContaining({ theme: 'dark' }) + ); + expect(createEditor).toHaveBeenCalledTimes(1); }); it('exposes the editor instance through the ref and calls onLoad', async () => { From 0e559ddbc55993f70145ae61f0c458157c082d34 Mon Sep 17 00:00:00 2001 From: sidgaikwad Date: Sat, 12 Sep 2026 14:08:39 +0530 Subject: [PATCH 5/5] fix: emit null for array holes and read toJSON once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two JSON.stringify parity bugs in stableKey. Array holes: .map() skips them, so a sparse array joined to fewer entries than its length. new Array(1) serialised to '[]' and collided with a genuinely empty array; new Array(3) gave '[,,]'. Iterate by index instead, so a hole reads as undefined and serialises to null, matching JSON.stringify exactly. toJSON: the property was read twice, once to type-check and once to call, so a toJSON defined as a getter ran twice. Read it once and invoke it with .call(value, key) — caching the method loses the `this` that method-call syntax bound for free, and implementations rely on it. --- src/stableKey.ts | 31 ++++++++++++++++++++++++------- test/stableKey.test.ts | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/src/stableKey.ts b/src/stableKey.ts index f28acac..0358028 100644 --- a/src/stableKey.ts +++ b/src/stableKey.ts @@ -26,14 +26,24 @@ const serialize = ( // symbol) where JSON.stringify itself returns undefined. if (value === null || typeof value !== 'object') return JSON.stringify(value); - const object = value as { toJSON?: (key: string) => unknown }; - if (honourToJSON && typeof object.toJSON === 'function') { - // Dispatch toJSON exactly once and serialize its result directly, as + // Read the property once. A toJSON defined as a getter would otherwise be + // invoked twice — once to type-check it, once to call it — so a getter with + // side effects, or one returning a fresh function each read, behaved + // differently here than under JSON.stringify. + const toJSON = honourToJSON + ? (value as { toJSON?: (key: string) => unknown }).toJSON + : undefined; + if (typeof toJSON === 'function') { + // .call, because caching the method above loses the `this` that + // method-call syntax bound for free. JSON.stringify invokes toJSON with + // `this` set to the value, and implementations rely on it. + // + // Dispatch exactly once and serialize the result directly, as // JSON.stringify does. Re-dispatching would let a toJSON that returns // `this` recurse until the stack overflows — during render, before the // cycle guard below is ever reached. Properties *inside* the result // still get their own dispatch, which is also what JSON.stringify does. - return serialize(object.toJSON(key), seen, key, false); + return serialize(toJSON.call(value, key), seen, key, false); } if (seen.has(value)) return '"[Circular]"'; @@ -41,9 +51,16 @@ const serialize = ( let result: string; if (Array.isArray(value)) { - result = `[${value - .map((item, index) => serialize(item, seen, String(index)) ?? 'null') - .join(',')}]`; + // An index loop, not .map(): .map() skips holes, so a sparse array + // joined to fewer entries than its length — new Array(1) serialized as + // '[]' and collided with a genuinely empty array. Reading value[index] + // yields undefined for a hole, which serializes to null, matching + // JSON.stringify. + const items: string[] = []; + for (let index = 0; index < value.length; index++) { + items.push(serialize(value[index], seen, String(index)) ?? 'null'); + } + result = `[${items.join(',')}]`; } else { const entries: string[] = []; for (const name of Object.keys(value).sort()) { diff --git a/test/stableKey.test.ts b/test/stableKey.test.ts index e5e712d..8246d50 100644 --- a/test/stableKey.test.ts +++ b/test/stableKey.test.ts @@ -130,3 +130,44 @@ it('still dispatches toJSON for properties inside a toJSON result', () => { expect(stableKey(outer)).toBe(JSON.stringify(outer)); expect(stableKey(outer)).toBe('{"nested":"INNER","plain":1}'); }); + +it('matches JSON.stringify for sparse arrays', () => { + // .map() skips holes, so a sparse array joined to fewer entries than its + // length: new Array(1) serialized as '[]' and collided with []. + expect(stableKey(new Array(1))).toBe(JSON.stringify(new Array(1))); + expect(stableKey(new Array(3))).toBe(JSON.stringify(new Array(3))); + expect(stableKey(new Array(1))).not.toBe(stableKey([])); + + const trailing = [1]; + trailing[3] = 4; + expect(stableKey(trailing)).toBe(JSON.stringify(trailing)); +}); + +it('reads a toJSON getter exactly once', () => { + // The property used to be read twice — once to type-check it, once to call + // it — so a getter ran twice. + let reads = 0; + const value = { + get toJSON() { + reads++; + return () => 'ok'; + }, + }; + + expect(stableKey(value)).toBe('"ok"'); + expect(reads).toBe(1); +}); + +it('invokes toJSON with `this` bound to the value', () => { + // Caching the method off the object loses the binding that method-call + // syntax gave for free, so it has to be re-applied with .call. + const value = { + marker: 'mine', + toJSON() { + return this.marker; + }, + }; + + expect(stableKey(value)).toBe(JSON.stringify(value)); + expect(stableKey(value)).toBe('"mine"'); +});