fix: compare options with an order-insensitive key

src/ImageEditor.tsx

@@ -2,20 +2,31 @@ 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,
   ref: React.Ref<ImageEditorRef>
 ) {
   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,

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<object>,
+  // 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<string, unknown>)[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';

test/index.test.tsx

@@ -9,13 +9,21 @@ 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', () => ({
   loadScript: vi.fn(() => Promise.resolve()),
   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<typeof import('../src/stableKey')>();
+  return { stableKey: vi.fn(actual.stableKey) };
+});
+
 interface Deferred<T> {
   promise: Promise<T>;
   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(
+    <ImageEditor image="img-a" options={{ projectId: 1234, offline: false }} />
+  );
+  await flush();
+
+  // Same configuration, keys in the other order — the common case when
+  // options are assembled conditionally rather than as one fixed literal.
+  rerender(
+    <ImageEditor image="img-a" options={{ offline: false, projectId: 1234 }} />
+  );
+  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(<ImageEditor image="img-a" options={options} />);
+  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(<ImageEditor image="img-a" options={options} />);
+  rerender(<ImageEditor image="img-b" options={options} />);
+  rerender(<ImageEditor image="img-c" options={options} />);
+  await flush();
+
+  expect(vi.mocked(stableKey).mock.calls.length).toBe(afterMount);
+});
+
+it('re-serializes when a fresh options object arrives', async () => {
+  const { rerender } = render(
+    <ImageEditor image="img-a" options={{ projectId: 1234 }} />
+  );
+  await flush();
+  const afterMount = vi.mocked(stableKey).mock.calls.length;
+
+  rerender(<ImageEditor image="img-a" options={{ projectId: 1234 }} />);
+
+  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(<ImageEditor image="img-a" />);
+  await flush();
+  const afterMount = vi.mocked(stableKey).mock.calls.length;
+
+  rerender(<ImageEditor image="img-b" />);
+  rerender(<ImageEditor image="img-c" />);
+  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<ImageEditorRef>();
   const onLoad = vi.fn();

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<string, unknown> = { 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}');
+});