From 96565b27cb4d1060a46fc5cead9de83fed15dc84 Mon Sep 17 00:00:00 2001 From: Duane Johnson Date: Tue, 26 Aug 2025 09:57:53 -0600 Subject: [PATCH 1/5] Add onOperation hook - Adds an onOperation hook that intercepts all proxy paths - Very minimal performance impact --- src/draft.ts | 6 +- src/interface.ts | 22 ++++ src/makeCreator.ts | 1 + src/map.ts | 8 +- src/set.ts | 30 +++-- src/utils/hooks.ts | 41 ++++++ src/utils/index.ts | 1 + test/hooks.test.ts | 305 +++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 402 insertions(+), 12 deletions(-) create mode 100644 src/utils/hooks.ts create mode 100644 test/hooks.test.ts diff --git a/src/draft.ts b/src/draft.ts index 2c99c6b2..d6442200 100644 --- a/src/draft.ts +++ b/src/draft.ts @@ -29,6 +29,7 @@ import { finalizeSetValue, markFinalization, finalizePatches, + _emitOp, } from './utils'; import { checkReadable } from './unsafe'; import { generatePatches } from './patch'; @@ -147,6 +148,7 @@ const proxyHandler: ProxyHandler = { return true; } const current = peek(latest(target), key); + _emitOp(target, key, { kind: 'set', key, prev: current, next: value }); const currentProxyDraft = getProxyDraft(current); if (currentProxyDraft && isEqual(currentProxyDraft.original, value)) { // !case: ignore the case of assigning the original draftable value to a draft @@ -203,7 +205,8 @@ const proxyHandler: ProxyHandler = { if (target.type === DraftType.Array) { return proxyHandler.set!.call(this, target, key, undefined, target.proxy); } - if (peek(target.original, key) !== undefined || key in target.original) { + const prev = peek(target.original, key); + if (prev !== undefined || key in target.original) { // !case: delete an existing key ensureShallowCopy(target); markChanged(target); @@ -214,6 +217,7 @@ const proxyHandler: ProxyHandler = { target.assignedMap.delete(key); } if (target.copy) delete target.copy[key]; + _emitOp(target, key, { kind: 'delete', key, prev }); return true; }, }; diff --git a/src/interface.ts b/src/interface.ts index d38d969a..84788bcd 100644 --- a/src/interface.ts +++ b/src/interface.ts @@ -109,6 +109,16 @@ export interface ApplyMutableOptions { mutable?: boolean; } +export interface OperationEvent { + kind: 'set' | 'delete' | 'map.set' | 'map.delete' | 'map.clear' | 'set.add' | 'set.delete' | 'set.clear'; + key?: any; + prev?: any; + next?: any; + value?: any; + existed?: boolean; + path: (string | number)[]; +} + export interface Options { /** * In strict mode, Forbid accessing non-draftable values and forbid returning a non-draft value. @@ -127,6 +137,12 @@ export interface Options { * And it can also return a shallow copy function(AutoFreeze and Patches should both be disabled). */ mark?: Mark; + /** + * Hooks for operation events. + */ + hooks?: { + onOperation?: (event: OperationEvent) => void; + }; } export interface ExternalOptions { @@ -147,6 +163,12 @@ export interface ExternalOptions { * And it can also return a shallow copy function(AutoFreeze and Patches should both be disabled). */ mark?: Mark[] | Mark; + /** + * Hooks for operation events. + */ + hooks?: { + onOperation?: (event: OperationEvent) => void; + }; } // Exclude `symbol` diff --git a/src/makeCreator.ts b/src/makeCreator.ts index e4e93881..cb65b300 100644 --- a/src/makeCreator.ts +++ b/src/makeCreator.ts @@ -148,6 +148,7 @@ export const makeCreator: MakeCreator = (arg) => { mark, strict, enablePatches, + hooks: options.hooks, }; if ( !isDraftable(state, _options) && diff --git a/src/map.ts b/src/map.ts index 52dbc2df..9d53de0b 100644 --- a/src/map.ts +++ b/src/map.ts @@ -10,6 +10,7 @@ import { latest, markChanged, markFinalization, + _emitOp, } from './utils'; export const mapHandler = { @@ -23,12 +24,14 @@ export const mapHandler = { set(key: any, value: any) { const target = getProxyDraft(this)!; const source = latest(target); - if (!source.has(key) || !isEqual(source.get(key), value)) { + const prev = source.get(key); + if (!source.has(key) || !isEqual(prev, value)) { ensureShallowCopy(target); markChanged(target); target.assignedMap!.set(key, true); target.copy.set(key, value); markFinalization(target, key, value, generatePatches); + _emitOp(target, undefined, { kind:'map.set', key, prev, next: value }); } return this; }, @@ -37,6 +40,7 @@ export const mapHandler = { return false; } const target = getProxyDraft(this)!; + const prev = latest(target).get(key); ensureShallowCopy(target); markChanged(target); if (target.original.has(key)) { @@ -45,6 +49,7 @@ export const mapHandler = { target.assignedMap!.delete(key); } target.copy.delete(key); + _emitOp(target, undefined, { kind:'map.delete', key, prev }); return true; }, clear() { @@ -57,6 +62,7 @@ export const mapHandler = { target.assignedMap.set(key, false); } target.copy!.clear(); + _emitOp(target, undefined, { kind:'map.clear' }); }, forEach(callback: (value: any, key: any, self: any) => void, thisArg?: any) { const target = getProxyDraft(this)!; diff --git a/src/set.ts b/src/set.ts index 824aa735..701e554d 100644 --- a/src/set.ts +++ b/src/set.ts @@ -7,6 +7,7 @@ import { isDraftable, markChanged, markFinalization, + _emitOp, } from './utils'; import { checkReadable } from './unsafe'; import { generatePatches } from './patch'; @@ -79,31 +80,39 @@ export const setHandler = { target.assignedMap!.set(value, true); target.setMap!.set(value, value); markFinalization(target, value, value, generatePatches); + _emitOp(target, undefined, { kind:'set.add', value }); } return this; }, delete(value: any): boolean { - if (!this.has(value)) { + const existed = this.has(value); + if (!existed) { return false; } const target = getProxyDraft(this)!; ensureShallowCopy(target); markChanged(target); const valueProxyDraft = getProxyDraft(value)!; + let ok: boolean; if (valueProxyDraft && target.setMap!.has(valueProxyDraft.original)) { // delete drafted target.assignedMap!.set(valueProxyDraft.original, false); - return target.setMap!.delete(valueProxyDraft.original); + ok = target.setMap!.delete(valueProxyDraft.original); } - if (!valueProxyDraft && target.setMap!.has(value)) { - // non-draftable values - target.assignedMap!.set(value, false); - } else { - // reassigned - target.assignedMap!.delete(value); + else { + if (!valueProxyDraft && target.setMap!.has(value)) { + // non-draftable values + target.assignedMap!.set(value, false); + } + else { + // reassigned + target.assignedMap!.delete(value); + } + // delete reassigned or non-draftable values + ok = target.setMap!.delete(value); } - // delete reassigned or non-draftable values - return target.setMap!.delete(value); + _emitOp(target, undefined, { kind:'set.delete', value, existed: ok || existed }); + return ok; }, clear() { if (!this.size) return; @@ -114,6 +123,7 @@ export const setHandler = { target.assignedMap!.set(value, false); } target.setMap!.clear(); + _emitOp(target, undefined, { kind:'set.clear' }); }, values(): IterableIterator { const target = getProxyDraft(this)!; diff --git a/src/utils/hooks.ts b/src/utils/hooks.ts new file mode 100644 index 00000000..99bfc0ab --- /dev/null +++ b/src/utils/hooks.ts @@ -0,0 +1,41 @@ +import { ProxyDraft, OperationEvent } from '../interface'; +import { ensureShallowCopy } from './copy'; + +/** + * Get the path to a proxy draft node + */ +function getPath(proxyDraft: ProxyDraft): (string | number)[] | null { + const path: (string | number)[] = []; + let current: ProxyDraft | null = proxyDraft; + + while (current && current.parent) { + if (current.key !== undefined) { + path.unshift(current.key as string | number); + } + current = current.parent; + } + + return path; +} + +/** + * Emit an operation event through the hooks system + */ +export function _emitOp(proxyDraft: ProxyDraft | null, key: any, evt: Omit): void { + const hooks = proxyDraft && proxyDraft.options && proxyDraft.options.hooks; + const onOperation = hooks && hooks.onOperation; + if (!onOperation) return; + + // Ensure path is computed against the *current* draft node. + // getPath(proxyDraft) returns the path to the container; append key when present. + try { + // ensure the container has a shallow copy so getPath() can validate + ensureShallowCopy(proxyDraft); + const basePath = getPath(proxyDraft); + if (!basePath) return; + const path = (key === undefined) ? basePath.slice() : basePath.concat([key]); + onOperation(Object.assign({ path }, evt)); + } catch (e) { + // Be conservative: never throw from hooks + } +} \ No newline at end of file diff --git a/src/utils/index.ts b/src/utils/index.ts index 057e62cb..d41e6536 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -5,3 +5,4 @@ export * from './draft'; export * from './proto'; export * from './forEach'; export * from './finalize'; +export * from './hooks'; diff --git a/test/hooks.test.ts b/test/hooks.test.ts new file mode 100644 index 00000000..621fe2db --- /dev/null +++ b/test/hooks.test.ts @@ -0,0 +1,305 @@ +/* eslint-disable no-param-reassign */ +import { create } from '../src'; + +describe('hooks', () => { + test('object operations with hooks', () => { + const operations: any[] = []; + const baseState: { + name: string; + age?: number; + } = { + name: 'Alice', + age: 30, + }; + + const state = create( + baseState, + (draft) => { + draft.name = 'Bob'; + draft.age = 31; + delete draft.age; + }, + { + hooks: { + onOperation: (event) => { + operations.push(event); + }, + }, + } + ); + + expect(state).toEqual({ name: 'Bob' }); + expect(operations).toHaveLength(3); + + // Check set operation + expect(operations[0]).toEqual({ + kind: 'set', + key: 'name', + prev: 'Alice', + next: 'Bob', + path: ['name'], + }); + + // Check set operation for age + expect(operations[1]).toEqual({ + kind: 'set', + key: 'age', + prev: 30, + next: 31, + path: ['age'], + }); + + // Check delete operation + expect(operations[2]).toEqual({ + kind: 'delete', + key: 'age', + prev: 30, // This captures the original value, not the modified value + path: ['age'], + }); + }); + + test('map operations with hooks', () => { + const operations: any[] = []; + const baseState = { + map: new Map([['key1', 'value1'], ['key2', 'value2']]), + }; + + const state = create( + baseState, + (draft) => { + draft.map.set('key3', 'value3'); + draft.map.set('key1', 'newValue1'); + draft.map.delete('key2'); + draft.map.clear(); + }, + { + hooks: { + onOperation: (event) => { + operations.push(event); + }, + }, + } + ); + + expect(state.map.size).toBe(0); + expect(operations).toHaveLength(4); + + // Check map.set operation (new key) + expect(operations[0]).toEqual({ + kind: 'map.set', + key: 'key3', + prev: undefined, + next: 'value3', + path: ['map'], + }); + + // Check map.set operation (existing key) + expect(operations[1]).toEqual({ + kind: 'map.set', + key: 'key1', + prev: 'value1', + next: 'newValue1', + path: ['map'], + }); + + // Check map.delete operation + expect(operations[2]).toEqual({ + kind: 'map.delete', + key: 'key2', + prev: 'value2', + path: ['map'], + }); + + // Check map.clear operation + expect(operations[3]).toEqual({ + kind: 'map.clear', + path: ['map'], + }); + }); + + test('set operations with hooks', () => { + const operations: any[] = []; + const baseState = { + set: new Set(['value1', 'value2']), + }; + + const state = create( + baseState, + (draft) => { + draft.set.add('value3'); + draft.set.add('value1'); // Should not trigger (already exists) + draft.set.delete('value2'); + draft.set.clear(); + }, + { + hooks: { + onOperation: (event) => { + operations.push(event); + }, + }, + } + ); + + expect(state.set.size).toBe(0); + expect(operations).toHaveLength(3); // add, delete, clear (no duplicate add) + + // Check set.add operation + expect(operations[0]).toEqual({ + kind: 'set.add', + value: 'value3', + path: ['set'], + }); + + // Check set.delete operation + expect(operations[1]).toEqual({ + kind: 'set.delete', + value: 'value2', + existed: true, + path: ['set'], + }); + + // Check set.clear operation + expect(operations[2]).toEqual({ + kind: 'set.clear', + path: ['set'], + }); + }); + + test('nested object operations with hooks', () => { + const operations: any[] = []; + const baseState: { + user: { + profile: { + name: string; + age?: number; + }; + }; + } = { + user: { + profile: { + name: 'Alice', + age: 30, + }, + }, + }; + + const state = create( + baseState, + (draft) => { + draft.user.profile.name = 'Bob'; + delete draft.user.profile.age; + }, + { + hooks: { + onOperation: (event) => { + operations.push(event); + }, + }, + } + ); + + expect(state.user.profile).toEqual({ name: 'Bob' }); + expect(operations).toHaveLength(2); + + // Check nested set operation + expect(operations[0]).toEqual({ + kind: 'set', + key: 'name', + prev: 'Alice', + next: 'Bob', + path: ['user', 'profile', 'name'], + }); + + // Check nested delete operation + expect(operations[1]).toEqual({ + kind: 'delete', + key: 'age', + prev: 30, + path: ['user', 'profile', 'age'], + }); + }); + + test('no hooks provided', () => { + const baseState = { name: 'Alice' }; + + // Should not throw when no hooks are provided + const state = create(baseState, (draft) => { + draft.name = 'Bob'; + }); + + expect(state).toEqual({ name: 'Bob' }); + }); + + test('hooks with error handling', () => { + const baseState = { name: 'Alice' }; + + // Should not throw even if hook throws an error + const state = create( + baseState, + (draft) => { + draft.name = 'Bob'; + }, + { + hooks: { + onOperation: () => { + throw new Error('Hook error'); + }, + }, + } + ); + + expect(state).toEqual({ name: 'Bob' }); + }); + + test('array operations with hooks', () => { + const operations: any[] = []; + const baseState = { + items: ['item1', 'item2'], + }; + + const state = create( + baseState, + (draft) => { + draft.items[0] = 'newItem1'; + draft.items.push('item3'); + }, + { + hooks: { + onOperation: (event) => { + operations.push(event); + }, + }, + } + ); + + expect(state.items).toEqual(['newItem1', 'item2', 'item3']); + expect(operations).toHaveLength(3); // array[0] set, array[2] set, length set + + // Check array set operation + expect(operations[0]).toEqual({ + kind: 'set', + key: '0', + prev: 'item1', + next: 'newItem1', + path: ['items', '0'], + }); + + // Check array push operation (sets new item) + expect(operations[1]).toEqual({ + kind: 'set', + key: '2', + prev: undefined, + next: 'item3', + path: ['items', '2'], + }); + + // Check array length update + expect(operations[2]).toEqual({ + kind: 'set', + key: 'length', + prev: 3, + next: 3, + path: ['items', 'length'], + }); + }); +}); \ No newline at end of file From 2e8378abd3628030690decbe7eed00b9f1d88b53 Mon Sep 17 00:00:00 2001 From: Duane Johnson Date: Tue, 26 Aug 2025 13:47:27 -0600 Subject: [PATCH 2/5] Use markChanged as interception point for onOperation --- src/draft.ts | 7 ++-- src/map.ts | 10 ++---- src/set.ts | 10 ++---- src/utils/hooks.ts | 41 ----------------------- src/utils/index.ts | 1 - src/utils/mark.ts | 37 ++++++++++++++++++++- test/hooks-external-api.test.ts | 59 +++++++++++++++++++++++++++++++++ test/hooks.test.ts | 17 +++------- 8 files changed, 107 insertions(+), 75 deletions(-) delete mode 100644 src/utils/hooks.ts create mode 100644 test/hooks-external-api.test.ts diff --git a/src/draft.ts b/src/draft.ts index d6442200..a22a7583 100644 --- a/src/draft.ts +++ b/src/draft.ts @@ -29,7 +29,6 @@ import { finalizeSetValue, markFinalization, finalizePatches, - _emitOp, } from './utils'; import { checkReadable } from './unsafe'; import { generatePatches } from './patch'; @@ -148,7 +147,6 @@ const proxyHandler: ProxyHandler = { return true; } const current = peek(latest(target), key); - _emitOp(target, key, { kind: 'set', key, prev: current, next: value }); const currentProxyDraft = getProxyDraft(current); if (currentProxyDraft && isEqual(currentProxyDraft.original, value)) { // !case: ignore the case of assigning the original draftable value to a draft @@ -164,7 +162,7 @@ const proxyHandler: ProxyHandler = { ) return true; ensureShallowCopy(target); - markChanged(target); + markChanged(target, key, { kind: 'set', prev: current, next: value }); if (has(target.original, key) && isEqual(value, target.original[key])) { // !case: handle the case of assigning the original non-draftable value to a draft target.assignedMap!.delete(key); @@ -209,7 +207,7 @@ const proxyHandler: ProxyHandler = { if (prev !== undefined || key in target.original) { // !case: delete an existing key ensureShallowCopy(target); - markChanged(target); + markChanged(target, key, { kind: 'delete', prev }); target.assignedMap!.set(key, false); } else { target.assignedMap = target.assignedMap ?? new Map(); @@ -217,7 +215,6 @@ const proxyHandler: ProxyHandler = { target.assignedMap.delete(key); } if (target.copy) delete target.copy[key]; - _emitOp(target, key, { kind: 'delete', key, prev }); return true; }, }; diff --git a/src/map.ts b/src/map.ts index 9d53de0b..f9aca49f 100644 --- a/src/map.ts +++ b/src/map.ts @@ -10,7 +10,6 @@ import { latest, markChanged, markFinalization, - _emitOp, } from './utils'; export const mapHandler = { @@ -27,11 +26,10 @@ export const mapHandler = { const prev = source.get(key); if (!source.has(key) || !isEqual(prev, value)) { ensureShallowCopy(target); - markChanged(target); + markChanged(target, key, { kind:'map.set', prev, next: value }); target.assignedMap!.set(key, true); target.copy.set(key, value); markFinalization(target, key, value, generatePatches); - _emitOp(target, undefined, { kind:'map.set', key, prev, next: value }); } return this; }, @@ -42,27 +40,25 @@ export const mapHandler = { const target = getProxyDraft(this)!; const prev = latest(target).get(key); ensureShallowCopy(target); - markChanged(target); + markChanged(target, key, { kind:'map.delete', prev }); if (target.original.has(key)) { target.assignedMap!.set(key, false); } else { target.assignedMap!.delete(key); } target.copy.delete(key); - _emitOp(target, undefined, { kind:'map.delete', key, prev }); return true; }, clear() { const target = getProxyDraft(this)!; if (!this.size) return; ensureShallowCopy(target); - markChanged(target); + markChanged(target, undefined, { kind:'map.clear' }); target.assignedMap = new Map(); for (const [key] of target.original) { target.assignedMap.set(key, false); } target.copy!.clear(); - _emitOp(target, undefined, { kind:'map.clear' }); }, forEach(callback: (value: any, key: any, self: any) => void, thisArg?: any) { const target = getProxyDraft(this)!; diff --git a/src/set.ts b/src/set.ts index 701e554d..30e0e1ae 100644 --- a/src/set.ts +++ b/src/set.ts @@ -7,7 +7,6 @@ import { isDraftable, markChanged, markFinalization, - _emitOp, } from './utils'; import { checkReadable } from './unsafe'; import { generatePatches } from './patch'; @@ -76,11 +75,10 @@ export const setHandler = { const target = getProxyDraft(this)!; if (!this.has(value)) { ensureShallowCopy(target); - markChanged(target); + markChanged(target, undefined, { kind:'set.add', value }); target.assignedMap!.set(value, true); target.setMap!.set(value, value); markFinalization(target, value, value, generatePatches); - _emitOp(target, undefined, { kind:'set.add', value }); } return this; }, @@ -91,7 +89,6 @@ export const setHandler = { } const target = getProxyDraft(this)!; ensureShallowCopy(target); - markChanged(target); const valueProxyDraft = getProxyDraft(value)!; let ok: boolean; if (valueProxyDraft && target.setMap!.has(valueProxyDraft.original)) { @@ -111,19 +108,18 @@ export const setHandler = { // delete reassigned or non-draftable values ok = target.setMap!.delete(value); } - _emitOp(target, undefined, { kind:'set.delete', value, existed: ok || existed }); + markChanged(target, undefined, { kind:'set.delete', value, existed: ok || existed }); return ok; }, clear() { if (!this.size) return; const target = getProxyDraft(this)!; ensureShallowCopy(target); - markChanged(target); + markChanged(target, undefined, { kind:'set.clear' }); for (const value of target.original) { target.assignedMap!.set(value, false); } target.setMap!.clear(); - _emitOp(target, undefined, { kind:'set.clear' }); }, values(): IterableIterator { const target = getProxyDraft(this)!; diff --git a/src/utils/hooks.ts b/src/utils/hooks.ts deleted file mode 100644 index 99bfc0ab..00000000 --- a/src/utils/hooks.ts +++ /dev/null @@ -1,41 +0,0 @@ -import { ProxyDraft, OperationEvent } from '../interface'; -import { ensureShallowCopy } from './copy'; - -/** - * Get the path to a proxy draft node - */ -function getPath(proxyDraft: ProxyDraft): (string | number)[] | null { - const path: (string | number)[] = []; - let current: ProxyDraft | null = proxyDraft; - - while (current && current.parent) { - if (current.key !== undefined) { - path.unshift(current.key as string | number); - } - current = current.parent; - } - - return path; -} - -/** - * Emit an operation event through the hooks system - */ -export function _emitOp(proxyDraft: ProxyDraft | null, key: any, evt: Omit): void { - const hooks = proxyDraft && proxyDraft.options && proxyDraft.options.hooks; - const onOperation = hooks && hooks.onOperation; - if (!onOperation) return; - - // Ensure path is computed against the *current* draft node. - // getPath(proxyDraft) returns the path to the container; append key when present. - try { - // ensure the container has a shallow copy so getPath() can validate - ensureShallowCopy(proxyDraft); - const basePath = getPath(proxyDraft); - if (!basePath) return; - const path = (key === undefined) ? basePath.slice() : basePath.concat([key]); - onOperation(Object.assign({ path }, evt)); - } catch (e) { - // Be conservative: never throw from hooks - } -} \ No newline at end of file diff --git a/src/utils/index.ts b/src/utils/index.ts index d41e6536..057e62cb 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -5,4 +5,3 @@ export * from './draft'; export * from './proto'; export * from './forEach'; export * from './finalize'; -export * from './hooks'; diff --git a/src/utils/mark.ts b/src/utils/mark.ts index 97ef44fd..a8575c95 100644 --- a/src/utils/mark.ts +++ b/src/utils/mark.ts @@ -1,6 +1,12 @@ import { ProxyDraft } from '../interface'; -export function markChanged(proxyDraft: ProxyDraft) { +export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symbol, operation?: { + kind: 'set' | 'delete' | 'map.set' | 'map.delete' | 'map.clear' | 'set.add' | 'set.delete' | 'set.clear'; + prev?: any; + next?: any; + value?: any; + existed?: boolean; +}) { proxyDraft.assignedMap = proxyDraft.assignedMap ?? new Map(); if (!proxyDraft.operated) { proxyDraft.operated = true; @@ -8,4 +14,33 @@ export function markChanged(proxyDraft: ProxyDraft) { markChanged(proxyDraft.parent); } } + + // Emit operation hook if provided + if (operation && proxyDraft.options.hooks?.onOperation) { + try { + // Build path from root to this node + const path: (string | number)[] = []; + let current: ProxyDraft | null = proxyDraft; + + while (current && current.parent) { + if (current.key !== undefined) { + path.unshift(current.key as string | number); + } + current = current.parent; + } + + // Add the current key if provided + if (key !== undefined) { + path.push(key as string | number); + } + + proxyDraft.options.hooks.onOperation({ + ...operation, + path, + key, + }); + } catch (e) { + // Be conservative: never throw from hooks + } + } } diff --git a/test/hooks-external-api.test.ts b/test/hooks-external-api.test.ts new file mode 100644 index 00000000..1b279716 --- /dev/null +++ b/test/hooks-external-api.test.ts @@ -0,0 +1,59 @@ +/* eslint-disable no-param-reassign */ +import { create } from '../src'; + +describe('hooks external API', () => { + test('hooks should work through external create API', () => { + const operations: any[] = []; + const baseState = { name: 'Alice', age: 30 }; + + // This should work but currently doesn't because hooks aren't properly propagated + const state = create( + baseState, + (draft) => { + draft.name = 'Bob'; + }, + { + hooks: { + onOperation: (event) => { + operations.push(event); + }, + }, + } + ); + + expect(state).toEqual({ name: 'Bob', age: 30 }); + expect(operations).toHaveLength(1); // This will fail if hooks aren't working + expect(operations[0]).toEqual({ + kind: 'set', + key: 'name', + prev: 'Alice', + next: 'Bob', + path: ['name'], + }); + }); + + test('hooks should work through makeCreator', () => { + const operations: any[] = []; + + // Test makeCreator with hooks + const createWithHooks = create; + + const baseState = { count: 0 }; + const state = createWithHooks( + baseState, + (draft) => { + draft.count = 1; + }, + { + hooks: { + onOperation: (event) => { + operations.push(event); + }, + }, + } + ); + + expect(state).toEqual({ count: 1 }); + expect(operations).toHaveLength(1); + }); +}); \ No newline at end of file diff --git a/test/hooks.test.ts b/test/hooks.test.ts index 621fe2db..102cf0ef 100644 --- a/test/hooks.test.ts +++ b/test/hooks.test.ts @@ -90,7 +90,7 @@ describe('hooks', () => { key: 'key3', prev: undefined, next: 'value3', - path: ['map'], + path: ['map', 'key3'], }); // Check map.set operation (existing key) @@ -99,7 +99,7 @@ describe('hooks', () => { key: 'key1', prev: 'value1', next: 'newValue1', - path: ['map'], + path: ['map', 'key1'], }); // Check map.delete operation @@ -107,7 +107,7 @@ describe('hooks', () => { kind: 'map.delete', key: 'key2', prev: 'value2', - path: ['map'], + path: ['map', 'key2'], }); // Check map.clear operation @@ -273,7 +273,7 @@ describe('hooks', () => { ); expect(state.items).toEqual(['newItem1', 'item2', 'item3']); - expect(operations).toHaveLength(3); // array[0] set, array[2] set, length set + expect(operations).toHaveLength(2); // array[0] set, array[2] set (length changes aren't captured by markChanged) // Check array set operation expect(operations[0]).toEqual({ @@ -292,14 +292,5 @@ describe('hooks', () => { next: 'item3', path: ['items', '2'], }); - - // Check array length update - expect(operations[2]).toEqual({ - kind: 'set', - key: 'length', - prev: 3, - next: 3, - path: ['items', 'length'], - }); }); }); \ No newline at end of file From 8fe14675deecb033fad851b85a3cea6a2885246f Mon Sep 17 00:00:00 2001 From: Duane Johnson Date: Tue, 26 Aug 2025 13:54:17 -0600 Subject: [PATCH 3/5] Fix tests and remove unnecessary comments --- src/set.ts | 2 +- src/utils/mark.ts | 10 ++-------- test/hooks-external-api.test.ts | 3 +-- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/set.ts b/src/set.ts index 30e0e1ae..92922fe9 100644 --- a/src/set.ts +++ b/src/set.ts @@ -89,6 +89,7 @@ export const setHandler = { } const target = getProxyDraft(this)!; ensureShallowCopy(target); + markChanged(target, undefined, { kind:'set.delete', value, existed }); const valueProxyDraft = getProxyDraft(value)!; let ok: boolean; if (valueProxyDraft && target.setMap!.has(valueProxyDraft.original)) { @@ -108,7 +109,6 @@ export const setHandler = { // delete reassigned or non-draftable values ok = target.setMap!.delete(value); } - markChanged(target, undefined, { kind:'set.delete', value, existed: ok || existed }); return ok; }, clear() { diff --git a/src/utils/mark.ts b/src/utils/mark.ts index a8575c95..a5914b93 100644 --- a/src/utils/mark.ts +++ b/src/utils/mark.ts @@ -1,12 +1,6 @@ -import { ProxyDraft } from '../interface'; +import { ProxyDraft, OperationEvent } from '../interface'; -export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symbol, operation?: { - kind: 'set' | 'delete' | 'map.set' | 'map.delete' | 'map.clear' | 'set.add' | 'set.delete' | 'set.clear'; - prev?: any; - next?: any; - value?: any; - existed?: boolean; -}) { +export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symbol, operation?: Omit) { proxyDraft.assignedMap = proxyDraft.assignedMap ?? new Map(); if (!proxyDraft.operated) { proxyDraft.operated = true; diff --git a/test/hooks-external-api.test.ts b/test/hooks-external-api.test.ts index 1b279716..06d1174a 100644 --- a/test/hooks-external-api.test.ts +++ b/test/hooks-external-api.test.ts @@ -6,7 +6,6 @@ describe('hooks external API', () => { const operations: any[] = []; const baseState = { name: 'Alice', age: 30 }; - // This should work but currently doesn't because hooks aren't properly propagated const state = create( baseState, (draft) => { @@ -22,7 +21,7 @@ describe('hooks external API', () => { ); expect(state).toEqual({ name: 'Bob', age: 30 }); - expect(operations).toHaveLength(1); // This will fail if hooks aren't working + expect(operations).toHaveLength(1); expect(operations[0]).toEqual({ kind: 'set', key: 'name', From fa8e0051ef9f1d05e1f6b561dc04949d73cdb819 Mon Sep 17 00:00:00 2001 From: Duane Johnson Date: Tue, 26 Aug 2025 14:03:57 -0600 Subject: [PATCH 4/5] Switch to onChange, separate ChangeInput from ChangeEvent types --- src/interface.ts | 11 +++++++---- src/utils/mark.ts | 8 ++++---- test/hooks-external-api.test.ts | 4 ++-- test/hooks.test.ts | 12 ++++++------ 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/interface.ts b/src/interface.ts index 84788bcd..91eb442a 100644 --- a/src/interface.ts +++ b/src/interface.ts @@ -109,13 +109,16 @@ export interface ApplyMutableOptions { mutable?: boolean; } -export interface OperationEvent { +export interface ChangeInput { kind: 'set' | 'delete' | 'map.set' | 'map.delete' | 'map.clear' | 'set.add' | 'set.delete' | 'set.clear'; - key?: any; prev?: any; next?: any; value?: any; existed?: boolean; +} + +export interface ChangeEvent extends ChangeInput { + key?: any; path: (string | number)[]; } @@ -141,7 +144,7 @@ export interface Options { * Hooks for operation events. */ hooks?: { - onOperation?: (event: OperationEvent) => void; + onChange?: (event: ChangeEvent) => void; }; } @@ -167,7 +170,7 @@ export interface ExternalOptions { * Hooks for operation events. */ hooks?: { - onOperation?: (event: OperationEvent) => void; + onChange?: (event: ChangeEvent) => void; }; } diff --git a/src/utils/mark.ts b/src/utils/mark.ts index a5914b93..f0b1db63 100644 --- a/src/utils/mark.ts +++ b/src/utils/mark.ts @@ -1,6 +1,6 @@ -import { ProxyDraft, OperationEvent } from '../interface'; +import { ProxyDraft, ChangeInput } from '../interface'; -export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symbol, operation?: Omit) { +export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symbol, operation?: ChangeInput) { proxyDraft.assignedMap = proxyDraft.assignedMap ?? new Map(); if (!proxyDraft.operated) { proxyDraft.operated = true; @@ -10,7 +10,7 @@ export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symb } // Emit operation hook if provided - if (operation && proxyDraft.options.hooks?.onOperation) { + if (operation && proxyDraft.options.hooks?.onChange) { try { // Build path from root to this node const path: (string | number)[] = []; @@ -28,7 +28,7 @@ export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symb path.push(key as string | number); } - proxyDraft.options.hooks.onOperation({ + proxyDraft.options.hooks.onChange({ ...operation, path, key, diff --git a/test/hooks-external-api.test.ts b/test/hooks-external-api.test.ts index 06d1174a..be50d0e7 100644 --- a/test/hooks-external-api.test.ts +++ b/test/hooks-external-api.test.ts @@ -13,7 +13,7 @@ describe('hooks external API', () => { }, { hooks: { - onOperation: (event) => { + onChange: (event) => { operations.push(event); }, }, @@ -45,7 +45,7 @@ describe('hooks external API', () => { }, { hooks: { - onOperation: (event) => { + onChange: (event) => { operations.push(event); }, }, diff --git a/test/hooks.test.ts b/test/hooks.test.ts index 102cf0ef..b9881fdb 100644 --- a/test/hooks.test.ts +++ b/test/hooks.test.ts @@ -21,7 +21,7 @@ describe('hooks', () => { }, { hooks: { - onOperation: (event) => { + onChange: (event) => { operations.push(event); }, }, @@ -74,7 +74,7 @@ describe('hooks', () => { }, { hooks: { - onOperation: (event) => { + onChange: (event) => { operations.push(event); }, }, @@ -133,7 +133,7 @@ describe('hooks', () => { }, { hooks: { - onOperation: (event) => { + onChange: (event) => { operations.push(event); }, }, @@ -191,7 +191,7 @@ describe('hooks', () => { }, { hooks: { - onOperation: (event) => { + onChange: (event) => { operations.push(event); }, }, @@ -241,7 +241,7 @@ describe('hooks', () => { }, { hooks: { - onOperation: () => { + onChange: () => { throw new Error('Hook error'); }, }, @@ -265,7 +265,7 @@ describe('hooks', () => { }, { hooks: { - onOperation: (event) => { + onChange: (event) => { operations.push(event); }, }, From ad44f7b20a258aeb0abd8e33ac6d875456aff19b Mon Sep 17 00:00:00 2001 From: Duane Johnson Date: Tue, 26 Aug 2025 14:27:34 -0600 Subject: [PATCH 5/5] Remove 'hooks' object, improve test names --- src/interface.ts | 12 +++---- src/makeCreator.ts | 2 +- src/utils/mark.ts | 4 +-- test/hooks-external-api.test.ts | 58 ------------------------------- test/hooks.test.ts | 61 ++++++++++----------------------- 5 files changed, 26 insertions(+), 111 deletions(-) delete mode 100644 test/hooks-external-api.test.ts diff --git a/src/interface.ts b/src/interface.ts index 91eb442a..1b2d50f1 100644 --- a/src/interface.ts +++ b/src/interface.ts @@ -141,11 +141,9 @@ export interface Options { */ mark?: Mark; /** - * Hooks for operation events. + * onChange callback. When set, caller can be aware of each change as it happens. */ - hooks?: { - onChange?: (event: ChangeEvent) => void; - }; + onChange?: (event: ChangeEvent) => void; } export interface ExternalOptions { @@ -167,11 +165,9 @@ export interface ExternalOptions { */ mark?: Mark[] | Mark; /** - * Hooks for operation events. + * onChange callback. When set, caller can be aware of each change as it happens. */ - hooks?: { - onChange?: (event: ChangeEvent) => void; - }; + onChange?: (event: ChangeEvent) => void; } // Exclude `symbol` diff --git a/src/makeCreator.ts b/src/makeCreator.ts index cb65b300..9ad98055 100644 --- a/src/makeCreator.ts +++ b/src/makeCreator.ts @@ -148,7 +148,7 @@ export const makeCreator: MakeCreator = (arg) => { mark, strict, enablePatches, - hooks: options.hooks, + onChange: options.onChange, }; if ( !isDraftable(state, _options) && diff --git a/src/utils/mark.ts b/src/utils/mark.ts index f0b1db63..4d985fe5 100644 --- a/src/utils/mark.ts +++ b/src/utils/mark.ts @@ -10,7 +10,7 @@ export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symb } // Emit operation hook if provided - if (operation && proxyDraft.options.hooks?.onChange) { + if (operation && proxyDraft.options.onChange) { try { // Build path from root to this node const path: (string | number)[] = []; @@ -28,7 +28,7 @@ export function markChanged(proxyDraft: ProxyDraft, key?: string | number | symb path.push(key as string | number); } - proxyDraft.options.hooks.onChange({ + proxyDraft.options.onChange({ ...operation, path, key, diff --git a/test/hooks-external-api.test.ts b/test/hooks-external-api.test.ts deleted file mode 100644 index be50d0e7..00000000 --- a/test/hooks-external-api.test.ts +++ /dev/null @@ -1,58 +0,0 @@ -/* eslint-disable no-param-reassign */ -import { create } from '../src'; - -describe('hooks external API', () => { - test('hooks should work through external create API', () => { - const operations: any[] = []; - const baseState = { name: 'Alice', age: 30 }; - - const state = create( - baseState, - (draft) => { - draft.name = 'Bob'; - }, - { - hooks: { - onChange: (event) => { - operations.push(event); - }, - }, - } - ); - - expect(state).toEqual({ name: 'Bob', age: 30 }); - expect(operations).toHaveLength(1); - expect(operations[0]).toEqual({ - kind: 'set', - key: 'name', - prev: 'Alice', - next: 'Bob', - path: ['name'], - }); - }); - - test('hooks should work through makeCreator', () => { - const operations: any[] = []; - - // Test makeCreator with hooks - const createWithHooks = create; - - const baseState = { count: 0 }; - const state = createWithHooks( - baseState, - (draft) => { - draft.count = 1; - }, - { - hooks: { - onChange: (event) => { - operations.push(event); - }, - }, - } - ); - - expect(state).toEqual({ count: 1 }); - expect(operations).toHaveLength(1); - }); -}); \ No newline at end of file diff --git a/test/hooks.test.ts b/test/hooks.test.ts index b9881fdb..72ac6dd3 100644 --- a/test/hooks.test.ts +++ b/test/hooks.test.ts @@ -1,8 +1,8 @@ /* eslint-disable no-param-reassign */ import { create } from '../src'; -describe('hooks', () => { - test('object operations with hooks', () => { +describe('onChange', () => { + test('object operations', () => { const operations: any[] = []; const baseState: { name: string; @@ -20,10 +20,8 @@ describe('hooks', () => { delete draft.age; }, { - hooks: { - onChange: (event) => { - operations.push(event); - }, + onChange: (event) => { + operations.push(event); }, } ); @@ -58,7 +56,7 @@ describe('hooks', () => { }); }); - test('map operations with hooks', () => { + test('map operations', () => { const operations: any[] = []; const baseState = { map: new Map([['key1', 'value1'], ['key2', 'value2']]), @@ -73,10 +71,8 @@ describe('hooks', () => { draft.map.clear(); }, { - hooks: { - onChange: (event) => { - operations.push(event); - }, + onChange: (event) => { + operations.push(event); }, } ); @@ -117,7 +113,7 @@ describe('hooks', () => { }); }); - test('set operations with hooks', () => { + test('set operations', () => { const operations: any[] = []; const baseState = { set: new Set(['value1', 'value2']), @@ -132,10 +128,8 @@ describe('hooks', () => { draft.set.clear(); }, { - hooks: { - onChange: (event) => { - operations.push(event); - }, + onChange: (event) => { + operations.push(event); }, } ); @@ -165,7 +159,7 @@ describe('hooks', () => { }); }); - test('nested object operations with hooks', () => { + test('nested object operations', () => { const operations: any[] = []; const baseState: { user: { @@ -190,10 +184,8 @@ describe('hooks', () => { delete draft.user.profile.age; }, { - hooks: { - onChange: (event) => { - operations.push(event); - }, + onChange: (event) => { + operations.push(event); }, } ); @@ -219,18 +211,7 @@ describe('hooks', () => { }); }); - test('no hooks provided', () => { - const baseState = { name: 'Alice' }; - - // Should not throw when no hooks are provided - const state = create(baseState, (draft) => { - draft.name = 'Bob'; - }); - - expect(state).toEqual({ name: 'Bob' }); - }); - - test('hooks with error handling', () => { + test('with error handling', () => { const baseState = { name: 'Alice' }; // Should not throw even if hook throws an error @@ -240,10 +221,8 @@ describe('hooks', () => { draft.name = 'Bob'; }, { - hooks: { - onChange: () => { - throw new Error('Hook error'); - }, + onChange: () => { + throw new Error('Hook error'); }, } ); @@ -251,7 +230,7 @@ describe('hooks', () => { expect(state).toEqual({ name: 'Bob' }); }); - test('array operations with hooks', () => { + test('array operations', () => { const operations: any[] = []; const baseState = { items: ['item1', 'item2'], @@ -264,10 +243,8 @@ describe('hooks', () => { draft.items.push('item3'); }, { - hooks: { - onChange: (event) => { - operations.push(event); - }, + onChange: (event) => { + operations.push(event); }, } );