diff --git a/eslint.config.mjs b/eslint.config.mjs index e036b33..1ac1216 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -52,6 +52,11 @@ export default [ }, { files: ['**/*.test.js', '**/*.test.mjs'], + languageOptions: { + globals: { + ...globals.mocha + } + }, rules: { 'no-unused-expressions': 'off' } diff --git a/package.json b/package.json index 701b83e..e902042 100644 --- a/package.json +++ b/package.json @@ -63,8 +63,8 @@ "build:umd": "rollup -c --environment target:umd", "build:types": "tsc -p tsconfig.build.json", "docs": "typedoc", - "lint": "eslint src rollup.config.mjs eslint.config.mjs", - "lint:fix": "eslint src rollup.config.mjs eslint.config.mjs --fix", + "lint": "eslint src test rollup.config.mjs eslint.config.mjs", + "lint:fix": "eslint src test rollup.config.mjs eslint.config.mjs --fix", "publint": "publint", "test": "mocha", "type:check": "tsc --noEmit", diff --git a/src/events.ts b/src/events.ts index f906165..080f953 100644 --- a/src/events.ts +++ b/src/events.ts @@ -35,26 +35,26 @@ export type HandleEvent = (arg1?: any, arg2?: any, arg3?: any, arg4?: any, arg5? * events.unbind('testEvent'); */ class Events { - private _suspendEvents: boolean = false; + private _additionalEmitters!: Events[]; - private _additionalEmitters: Events[] = []; + private _events!: Record; - private _events: Record = {}; + private _suspendEvents!: boolean; /** * Creates a new Events instance. */ constructor() { - // _world - Object.defineProperty( - this, - '_events', { - enumerable: false, - configurable: false, - writable: true, - value: { } - } - ); + // Make internal properties non-enumerable so they don't get serialized + // when the object is converted to JSON (e.g., for ShareDB sync) + const props: [string, any][] = [ + ['_additionalEmitters', []], + ['_events', {}], + ['_suspendEvents', false] + ]; + for (const [name, value] of props) { + Object.defineProperty(this, name, { enumerable: false, writable: true, value }); + } } /** diff --git a/src/observer-list.ts b/src/observer-list.ts index 1bb89e0..3588801 100644 --- a/src/observer-list.ts +++ b/src/observer-list.ts @@ -7,7 +7,7 @@ import { Observer } from './observer'; class ObserverList extends Events { data: any[] = []; - private _indexed: Record = {}; + private _indexed!: Record; sorted: ((arg0: any, arg1: any) => number) | null = null; @@ -20,6 +20,9 @@ class ObserverList extends Events { constructor(options: { sorted?: (arg0: any, arg1: any) => number, index?: string } = {}) { super(); + // Make internal property non-enumerable so it doesn't get serialized + Object.defineProperty(this, '_indexed', { enumerable: false, writable: true, value: {} }); + this.sorted = options.sorted || null; this.index = options.index || null; } diff --git a/src/observer.ts b/src/observer.ts index 7477101..eb8b52b 100644 --- a/src/observer.ts +++ b/src/observer.ts @@ -36,27 +36,27 @@ export type ObserverSync = Events & { * observer.set('name', 'Jane'); // Logs: Name changed from John to Jane */ class Observer extends Events { - private _destroyed: boolean = false; + private _destroyed!: boolean; - private _path: string = ''; + private _path!: string; - private _keys: string[] = []; + private _keys!: string[]; - private _data: any = { }; + private _data!: any; - private _pathsWithDuplicates: any = null; + private _pathsWithDuplicates!: Set | null; - private _parent: Observer = null; + private _parent!: Observer | null; - private _parentPath: string = ''; + private _parentPath!: string; - private _parentField: any = null; + private _parentField!: any; - private _parentKey: any = null; + private _parentKey!: any; - private _latestFn: Function = null; + private _latestFn!: Function | null; - private _silent: boolean = false; + private _silent!: boolean; history: ObserverHistory; @@ -80,28 +80,27 @@ class Observer extends Events { } = {}) { super(); - // array paths where duplicate entries are allowed - normally - // we check if an array already has a value before inserting it - // but if the array path is in here we'll allow it - this._pathsWithDuplicates = null; - if (options.pathsWithDuplicates) { - this._pathsWithDuplicates = {}; - for (let i = 0; i < options.pathsWithDuplicates.length; i++) { - this._pathsWithDuplicates[options.pathsWithDuplicates[i]] = true; - } + // Make internal properties non-enumerable so they don't get serialized + // when the object is converted to JSON (e.g., for ShareDB sync) + const props: [string, any][] = [ + ['_destroyed', false], + ['_path', ''], + ['_keys', []], + ['_data', {}], + ['_pathsWithDuplicates', options.pathsWithDuplicates ? new Set(options.pathsWithDuplicates) : null], + ['_parent', options.parent || null], + ['_parentPath', options.parentPath || ''], + ['_parentField', options.parentField || null], + ['_parentKey', options.parentKey || null], + ['_latestFn', options.latestFn || null], + ['_silent', false] + ]; + for (const [name, value] of props) { + Object.defineProperty(this, name, { enumerable: false, writable: true, value }); } this.patch(data); - this._parent = options.parent || null; - this._parentPath = options.parentPath || ''; - this._parentField = options.parentField || null; - this._parentKey = options.parentKey || null; - - this._latestFn = options.latestFn || null; - - this._silent = false; - const propagate = function (evt: string) { return function (path: string, arg1: any, arg2: any, arg3: any) { if (!this._parent) { @@ -893,7 +892,7 @@ class Observer extends Events { } const path = node._path ? `${node._path}.${key}` : key; - if (value !== null && !allowDuplicates && (!this._pathsWithDuplicates || !this._pathsWithDuplicates[path])) { + if (value !== null && !allowDuplicates && !this._pathsWithDuplicates?.has(path)) { if (arr.indexOf(value) !== -1) { return; } diff --git a/test/events.test.mjs b/test/events.test.mjs new file mode 100644 index 0000000..8f62720 --- /dev/null +++ b/test/events.test.mjs @@ -0,0 +1,29 @@ +import { expect } from 'chai'; + +import { Events } from '../dist/index.mjs'; + +describe('Events', () => { + + it('has non-enumerable internal properties', () => { + const events = new Events(); + const keys = Object.keys(events); + + expect(keys).to.not.include('_events'); + expect(keys).to.not.include('_suspendEvents'); + expect(keys).to.not.include('_additionalEmitters'); + }); + + it('can be serialized to JSON without circular reference errors', () => { + const events1 = new Events(); + const events2 = new Events(); + + // Create a scenario that would cause circular reference if _additionalEmitters was enumerable + events1.addEmitter(events2); + events2.addEmitter(events1); + + // This should not throw + expect(() => JSON.stringify(events1)).to.not.throw(); + expect(() => JSON.stringify(events2)).to.not.throw(); + }); + +}); diff --git a/test/observer-list.test.mjs b/test/observer-list.test.mjs new file mode 100644 index 0000000..ffe6df0 --- /dev/null +++ b/test/observer-list.test.mjs @@ -0,0 +1,28 @@ +import { expect } from 'chai'; + +import { Observer, ObserverList } from '../dist/index.mjs'; + +describe('ObserverList', () => { + + it('has non-enumerable internal properties', () => { + const list = new ObserverList(); + const keys = Object.keys(list); + + expect(keys).to.not.include('_indexed'); + expect(keys).to.not.include('_events'); + expect(keys).to.not.include('_suspendEvents'); + expect(keys).to.not.include('_additionalEmitters'); + }); + + it('can be serialized to JSON without circular reference errors', () => { + const list = new ObserverList(); + const observer = new Observer({ id: 1, name: 'test' }); + list.add(observer); + + // This should not throw + expect(() => JSON.stringify(list.json())).to.not.throw(); + + observer.destroy(); + }); + +}); diff --git a/test/observer.test.mjs b/test/observer.test.mjs index 0171a79..6a51308 100644 --- a/test/observer.test.mjs +++ b/test/observer.test.mjs @@ -2,7 +2,6 @@ import { expect } from 'chai'; import { Observer } from '../dist/index.mjs'; - const getData = () => { return { name: 'Will', @@ -150,4 +149,93 @@ describe('Observer', () => { observer.destroy(); }); + + it('has non-enumerable internal properties', () => { + const observer = new Observer(getData()); + const keys = Object.keys(observer); + + // Events properties + expect(keys).to.not.include('_events'); + expect(keys).to.not.include('_suspendEvents'); + expect(keys).to.not.include('_additionalEmitters'); + + // Observer's own properties + expect(keys).to.not.include('_destroyed'); + expect(keys).to.not.include('_path'); + expect(keys).to.not.include('_keys'); + expect(keys).to.not.include('_data'); + expect(keys).to.not.include('_parent'); + expect(keys).to.not.include('_parentPath'); + expect(keys).to.not.include('_parentField'); + expect(keys).to.not.include('_parentKey'); + expect(keys).to.not.include('_latestFn'); + expect(keys).to.not.include('_silent'); + expect(keys).to.not.include('_pathsWithDuplicates'); + + observer.destroy(); + }); + + it('can be serialized to JSON without including internal properties', () => { + const observer = new Observer(getData()); + const jsonString = JSON.stringify(observer.json()); + + expect(jsonString).to.not.include('_events'); + expect(jsonString).to.not.include('_suspendEvents'); + expect(jsonString).to.not.include('_additionalEmitters'); + expect(jsonString).to.not.include('_parent'); + expect(jsonString).to.not.include('_data'); + + observer.destroy(); + }); + + it('can be serialized when nested Observers have parent references', () => { + // This tests the exact scenario from the bug: nested Observers with _parent references + const parent = new Observer({ + entries: [] + }); + + // Insert an object which becomes a nested Observer with _parent pointing back to parent + // This creates a true circular reference: + // - parent._data.entries[0] -> child Observer + // - child._parent -> parent + parent.insert('entries', { name: 'child' }); + + // The nested Observer has a reference back to parent - this would cause circular reference + // if _parent was enumerable + expect(() => JSON.stringify(parent.json())).to.not.throw(); + + // Verify the structure is correct + const json = parent.json(); + expect(json.entries).to.have.lengthOf(1); + expect(json.entries[0].name).to.equal('child'); + + parent.destroy(); + }); + + it('prevents duplicate array insertions by default', () => { + const observer = new Observer({ items: [] }); + + observer.insert('items', 'a'); + observer.insert('items', 'b'); + observer.insert('items', 'a'); // duplicate - should be ignored + + expect(observer.get('items')).to.deep.equal(['a', 'b']); + + observer.destroy(); + }); + + it('allows duplicate array insertions when path is in pathsWithDuplicates', () => { + const observer = new Observer({ items: [] }, { + pathsWithDuplicates: ['items'] + }); + + observer.insert('items', 'a'); + observer.insert('items', 'b'); + observer.insert('items', 'a'); // duplicate - should be allowed + + expect(observer.get('items')).to.deep.equal(['a', 'b', 'a']); + + observer.destroy(); + }); + });