diff --git a/src/ImageEditor.tsx b/src/ImageEditor.tsx index acae488..926175b 100644 --- a/src/ImageEditor.tsx +++ b/src/ImageEditor.tsx @@ -2,12 +2,23 @@ import React, { useEffect, useId, useImperativeHandle, + useMemo, useRef, useState, } from 'react'; import { loadScript, resetLoader } from './loadScript'; -import { ImageEditorInstance, ImageEditorProps, ImageEditorRef } from './types'; +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 = {}; function ImageEditorInner( props: ImageEditorProps, @@ -15,7 +26,7 @@ function ImageEditorInner( ) { const { image, - options = {}, + options = NO_OPTIONS, scriptUrl, minHeight = 500, style = {}, @@ -74,8 +85,23 @@ 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. + // + // 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; @@ -122,7 +148,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..f28acac --- /dev/null +++ b/src/stableKey.ts @@ -0,0 +1,70 @@ +/** + * 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, + // 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}"`; + + // 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?: (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(key), seen, key, false); + } + + if (seen.has(value)) return '"[Circular]"'; + seen.add(value); + + let result: string; + if (Array.isArray(value)) { + result = `[${value + .map((item, index) => serialize(item, seen, String(index)) ?? 'null') + .join(',')}]`; + } else { + const entries: string[] = []; + for (const name of Object.keys(value).sort()) { + const serialized = serialize( + (value as Record)[name], + seen, + name + ); + if (serialized !== undefined) { + entries.push(`${JSON.stringify(name)}:${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..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 = { @@ -188,6 +197,67 @@ 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('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 new file mode 100644 index 0000000..e5e712d --- /dev/null +++ b/test/stableKey.test.ts @@ -0,0 +1,132 @@ +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, 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('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 }) }; + + expect(stableKey(outer)).toBe(JSON.stringify(outer)); + expect(stableKey(outer)).toBe('{"nested":"INNER","plain":1}'); +});