diff --git a/src/ImageEditor.tsx b/src/ImageEditor.tsx index acae488..c6c994a 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,18 @@ 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. + // + // 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; @@ -122,7 +133,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..0358028 --- /dev/null +++ b/src/stableKey.ts @@ -0,0 +1,87 @@ +/** + * 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); + + // 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(toJSON.call(value, key), seen, key, false); + } + + if (seen.has(value)) return '"[Circular]"'; + seen.add(value); + + let result: string; + if (Array.isArray(value)) { + // 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()) { + 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..706b7ed 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,72 @@ 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('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('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(); + + 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(); + + expect(mockInstance.updateOptions).toHaveBeenCalledWith( + expect.objectContaining({ theme: 'dark' }) + ); + 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..8246d50 --- /dev/null +++ b/test/stableKey.test.ts @@ -0,0 +1,173 @@ +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}'); +}); + +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"'); +});