diff --git a/packages/bindx-react/src/hooks/useEntity.ts b/packages/bindx-react/src/hooks/useEntity.ts index e750dcb7..5331ef23 100644 --- a/packages/bindx-react/src/hooks/useEntity.ts +++ b/packages/bindx-react/src/hooks/useEntity.ts @@ -333,11 +333,10 @@ export function useEntity( [rawHandle], ) - useEffect(() => { - return () => { - rawHandle.dispose() - } - }, [rawHandle]) + // `snapshot` is a useMemo dep, so `rawHandle` is recreated on every entity data change (to give + // memoized children a fresh reference). Superseded handles need no cleanup: EntityHandle is a + // stateless live view that owns no resources (see BaseHandle), so it is reclaimed by GC once + // unreferenced and a handle a consumer still holds stays usable for late reads/writes. // --- Persist & reset callbacks --- const persist = useCallback(async () => { diff --git a/packages/bindx-react/src/jsx/components/Entity.tsx b/packages/bindx-react/src/jsx/components/Entity.tsx index f229ca83..acdee09d 100644 --- a/packages/bindx-react/src/jsx/components/Entity.tsx +++ b/packages/bindx-react/src/jsx/components/Entity.tsx @@ -320,7 +320,12 @@ function EntityHandleRenderer({ [rawHandle], ) - useEffect(() => () => { rawHandle.dispose() }, [rawHandle]) + // `version` is a useMemo dep, so `rawHandle` is recreated on every entity data change (to give + // memoized children a fresh reference). Superseded handles need no cleanup: EntityHandle and its + // child Field/HasMany handles are stateless live views that own no resources (see BaseHandle), + // so they are reclaimed by GC once unreferenced — and a consumer that still holds an accessor + // from an earlier render (e.g. the Slate block editor dispatching `setValue` from an async + // onChange after the version bumped) can keep using it. const result = children(handle as EntityAccessor) return <>{annotateElement(result, { 'data-entity': entityType, 'data-entity-id': entityId })} diff --git a/packages/bindx/src/core/Disposable.ts b/packages/bindx/src/core/Disposable.ts deleted file mode 100644 index 6f9acca9..00000000 --- a/packages/bindx/src/core/Disposable.ts +++ /dev/null @@ -1,126 +0,0 @@ -/** - * Disposable pattern for cleanup management. - * Ensures proper cleanup of subscriptions and resources. - */ - -/** - * Interface for objects that can be disposed. - */ -export interface Disposable { - dispose(): void -} - -/** - * A group of disposables that can be disposed together. - * Provides safe cleanup even if individual dispose() calls throw. - */ -export class DisposableGroup implements Disposable { - private disposables: Disposable[] = [] - private disposed = false - - /** - * Adds a disposable to the group. - * If the group is already disposed, the disposable is disposed immediately. - */ - add(disposable: Disposable): void { - if (this.disposed) { - // Already disposed, dispose immediately - try { - disposable.dispose() - } catch (error) { - console.error('Error disposing:', error) - } - return - } - this.disposables.push(disposable) - } - - /** - * Adds a callback as a disposable. - */ - addCallback(callback: () => void): void { - this.add({ dispose: callback }) - } - - /** - * Adds an unsubscribe function as a disposable. - */ - addUnsubscribe(unsubscribe: () => void): void { - this.addCallback(unsubscribe) - } - - /** - * Disposes all items in the group. - * Each item is disposed even if previous ones throw. - */ - dispose(): void { - if (this.disposed) return - this.disposed = true - - const errors: Error[] = [] - - for (const disposable of this.disposables) { - try { - disposable.dispose() - } catch (error) { - errors.push(error instanceof Error ? error : new Error(String(error))) - } - } - - this.disposables = [] - - if (errors.length > 0) { - console.error('Errors during disposal:', errors) - } - } - - /** - * Checks if the group has been disposed. - */ - isDisposed(): boolean { - return this.disposed - } - - /** - * Gets the number of disposables in the group. - */ - get size(): number { - return this.disposables.length - } -} - -/** - * Creates a disposable from a callback function. - */ -export function createDisposable(callback: () => void): Disposable { - let disposed = false - return { - dispose() { - if (disposed) return - disposed = true - callback() - }, - } -} - -/** - * Combines multiple disposables into one. - */ -export function combineDisposables(...disposables: Disposable[]): Disposable { - const group = new DisposableGroup() - for (const d of disposables) { - group.add(d) - } - return group -} - -/** - * Type guard to check if something is disposable. - */ -export function isDisposable(value: unknown): value is Disposable { - return ( - typeof value === 'object' && - value !== null && - typeof (value as Disposable).dispose === 'function' - ) -} diff --git a/packages/bindx/src/handles/BaseHandle.ts b/packages/bindx/src/handles/BaseHandle.ts index 6dbd5ae3..6467926a 100644 --- a/packages/bindx/src/handles/BaseHandle.ts +++ b/packages/bindx/src/handles/BaseHandle.ts @@ -1,19 +1,21 @@ -import type { Disposable, DisposableGroup } from '../core/Disposable.js' import type { ActionDispatcher } from '../core/ActionDispatcher.js' import type { SnapshotStore } from '../store/SnapshotStore.js' /** * Base class for all handles. - * Provides common functionality for subscription and disposal. * - * Handles are stable objects that: + * Handles are stateless live views over the store: * - Have stable identity (same instance across renders) - * - Provide subscribe/getSnapshot for useSyncExternalStore + * - Provide subscribe/getVersion for useSyncExternalStore * - Dispatch actions for mutations + * + * They own no resources — the only subscriptions a handle creates are returned + * from `subscribe()` and owned by `useSyncExternalStore`, not stored on the + * handle. There is therefore intentionally no dispose lifecycle: a handle is + * reclaimed by GC once unreferenced, and a superseded handle (one replaced by a + * fresh instance on a data change) stays fully usable for late reads/writes. */ -export abstract class BaseHandle implements Disposable { - protected _disposed = false - +export abstract class BaseHandle { constructor( protected readonly store: SnapshotStore, protected readonly dispatcher: ActionDispatcher, @@ -28,29 +30,6 @@ export abstract class BaseHandle implements Disposable { * Get current version for change detection. */ abstract getVersion(): number - - /** - * Dispose resources (unsubscribe, cleanup). - */ - dispose(): void { - this._disposed = true - } - - /** - * Check if the handle has been disposed. - */ - get isDisposed(): boolean { - return this._disposed - } - - /** - * Throws if the handle has been disposed. - */ - protected assertNotDisposed(): void { - if (this._disposed) { - throw new Error('Handle has been disposed') - } - } } /** diff --git a/packages/bindx/src/handles/EntityHandle.ts b/packages/bindx/src/handles/EntityHandle.ts index d5db13a5..6cd8a47f 100644 --- a/packages/bindx/src/handles/EntityHandle.ts +++ b/packages/bindx/src/handles/EntityHandle.ts @@ -29,11 +29,10 @@ import { createHandleProxy } from './proxyFactory.js' import type { SelectionMeta } from '../selection/types.js' import { UnfetchedFieldError } from '../errors/UnfetchedFieldError.js' -/** Minimal internal interface for cached relation handles that need reset/dispose. - * At runtime, the proxied handles satisfy this through delegation, even if the public type doesn't expose dispose(). */ +/** Minimal internal interface for cached relation handles that need reset. + * At runtime, the proxied handles satisfy this through delegation. */ interface RelationHandleRaw { reset(): void - dispose(): void } interface CachedFieldHandle { @@ -428,7 +427,6 @@ export class EntityHandle extends Enti * Also resets all relation states (hasOne, hasMany). */ reset(): void { - this.assertNotDisposed() this.dispatcher.dispatch(resetEntity(this.entityType, this.entityId)) // Reset all cached relation handles @@ -441,27 +439,9 @@ export class EntityHandle extends Enti * Commits changes (serverData = data). */ commit(): void { - this.assertNotDisposed() this.dispatcher.dispatch(commitEntity(this.entityType, this.entityId)) } - /** - * Disposes the handle and all cached child handles. - */ - override dispose(): void { - super.dispose() - - for (const { raw } of this.fieldHandleCache.values()) { - raw.dispose() - } - this.fieldHandleCache.clear() - - for (const { raw } of this.relationHandleCache.values()) { - raw.dispose() - } - this.relationHandleCache.clear() - } - /** * Creates a proxy for field access. * Returns appropriate handle type based on schema field definition: @@ -541,7 +521,6 @@ export class EntityHandle extends Enti * Adds a client-side error to this entity. */ addError(error: ErrorInput): void { - this.assertNotDisposed() this.dispatcher.dispatch( addEntityError(this.entityType, this.entityId, createClientError(error)), ) @@ -551,7 +530,6 @@ export class EntityHandle extends Enti * Clears entity-level errors. */ clearErrors(): void { - this.assertNotDisposed() this.dispatcher.dispatch( clearEntityErrors(this.entityType, this.entityId), ) @@ -561,7 +539,6 @@ export class EntityHandle extends Enti * Clears all errors (entity-level, fields, and relations). */ clearAllErrors(): void { - this.assertNotDisposed() this.dispatcher.dispatch( clearAllErrorsAction(this.entityType, this.entityId), ) diff --git a/packages/bindx/src/handles/FieldHandle.ts b/packages/bindx/src/handles/FieldHandle.ts index d2cdcde9..5e223e59 100644 --- a/packages/bindx/src/handles/FieldHandle.ts +++ b/packages/bindx/src/handles/FieldHandle.ts @@ -124,7 +124,6 @@ export class FieldHandle extends EntityRelatedHandle { * Call this on blur or other interaction events. */ touch(): void { - this.assertNotDisposed() this.store.setFieldTouched(this.entityType, this.entityId, this.fieldName, true) } @@ -133,7 +132,6 @@ export class FieldHandle extends EntityRelatedHandle { * Also clears non-sticky client errors. */ setValue(value: T | null): void { - this.assertNotDisposed() // Clear non-sticky errors when value changes this.store.clearNonStickyFieldErrors(this.entityType, this.entityId, this.fieldName) this.dispatcher.dispatch( @@ -159,7 +157,6 @@ export class FieldHandle extends EntityRelatedHandle { * Adds a client-side error to this field. */ addError(error: ErrorInput): void { - this.assertNotDisposed() this.dispatcher.dispatch( addFieldError(this.entityType, this.entityId, this.fieldName, createClientError(error)), ) @@ -169,7 +166,6 @@ export class FieldHandle extends EntityRelatedHandle { * Clears all errors from this field. */ clearErrors(): void { - this.assertNotDisposed() this.dispatcher.dispatch( clearFieldErrors(this.entityType, this.entityId, this.fieldName), ) diff --git a/packages/bindx/src/handles/HasManyListHandle.ts b/packages/bindx/src/handles/HasManyListHandle.ts index 6ab29ce6..ae1f3e22 100644 --- a/packages/bindx/src/handles/HasManyListHandle.ts +++ b/packages/bindx/src/handles/HasManyListHandle.ts @@ -340,7 +340,6 @@ export class HasManyListHandle): string { - this.assertNotDisposed() // Pre-create entity so we can return tempId const tempId = this.store.createEntity(this.itemType, data as Record) @@ -399,7 +395,6 @@ export class HasManyListHandle is not assignable to HasManyRef. * This is a phantom property that only exists in the type system. @@ -489,7 +469,6 @@ export class HasManyListHandle * Accessible via proxy as `$create()`. */ create(data?: Partial): string { - this.assertNotDisposed() const tempId = this.store.createEntity(this.targetType, data as Record) this.dispatcher.dispatch( connectRelation(this.entityType, this.entityId, this.fieldName, tempId, this.targetType), @@ -386,7 +385,6 @@ export class HasOneHandle * Connects the relation to an entity. */ connect(targetId: string): void { - this.assertNotDisposed() this.dispatcher.dispatch( connectRelation(this.entityType, this.entityId, this.fieldName, targetId, this.targetType), ) @@ -397,7 +395,6 @@ export class HasOneHandle * Sets the FK to null — only works when the FK column is nullable. */ disconnect(): void { - this.assertNotDisposed() this.dispatcher.dispatch( disconnectRelation(this.entityType, this.entityId, this.fieldName), ) @@ -407,7 +404,6 @@ export class HasOneHandle * Marks the related entity for deletion. */ delete(): void { - this.assertNotDisposed() this.dispatcher.dispatch( deleteRelation(this.entityType, this.entityId, this.fieldName), ) @@ -432,22 +428,9 @@ export class HasOneHandle * Resets the relation to server state. */ reset(): void { - this.assertNotDisposed() this.store.resetRelation(this.entityType, this.entityId, this.fieldName) } - /** - * Disposes the handle. - */ - override dispose(): void { - super.dispose() - this.entityHandleCacheRaw?.dispose() - this.entityHandleCacheRaw = null - this.entityHandleCacheProxy = null - this.placeholderCacheRaw = null - this.placeholderCacheProxy = null - } - /** * Type brand - ensures HasOneRef is not assignable to HasOneRef. * This is a phantom property that only exists in the type system. @@ -474,7 +457,6 @@ export class HasOneHandle * Adds a client-side error to this relation. */ addError(error: ErrorInput): void { - this.assertNotDisposed() this.dispatcher.dispatch( addRelationError(this.entityType, this.entityId, this.fieldName, createClientError(error)), ) @@ -484,7 +466,6 @@ export class HasOneHandle * Clears all errors from this relation. */ clearErrors(): void { - this.assertNotDisposed() this.dispatcher.dispatch( clearRelationErrors(this.entityType, this.entityId, this.fieldName), ) diff --git a/tests/react/jsx/entityHandleDisposalAcrossRerender.test.tsx b/tests/react/jsx/entityHandleDisposalAcrossRerender.test.tsx new file mode 100644 index 00000000..25c8e06f --- /dev/null +++ b/tests/react/jsx/entityHandleDisposalAcrossRerender.test.tsx @@ -0,0 +1,63 @@ +import '../../setup' +import { describe, test, expect, afterEach } from 'bun:test' +import { render, waitFor, act, cleanup } from '@testing-library/react' +import React, { useRef } from 'react' +import { BindxProvider, MockAdapter, Entity } from '@contember/bindx-react' +import type { FieldAccessor } from '@contember/bindx' +import { getByTestId, queryByTestId, createMockData, schema, testSchema } from '../../shared' + +afterEach(() => { + cleanup() +}) + +describe(' accessor lifetime across data-driven re-render', () => { + // Models the editor integration: a consumer (e.g. the Slate BlockEditor) captures a field + // accessor handed to it during render and writes to it again on a later event. Between those + // writes the entity version changes (the first write itself bumps it), which makes + // EntityHandleRenderer recreate the EntityHandle and dispose the previous one — disposing the + // very accessor the consumer still holds. The second write must still succeed: dispose() frees + // no real resources, so a captured accessor for a still-mounted entity must remain writable. + test('a captured field accessor stays writable after the entity re-renders', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + + const captured = { field: null as FieldAccessor | null } + + function Editor({ title }: { title: FieldAccessor }) { + // Capture once, like Slate captures its onChange/field at mount time. + const ref = useRef | null>(null) + if (!ref.current) { + ref.current = title + captured.field = title + } + return {title.value} + } + + const { container } = render( + + + {article => } />} + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'title')).not.toBeNull() + }) + + // First write through the captured accessor: succeeds and bumps the entity version, + // which triggers EntityHandleRenderer to recreate (and dispose) the handle. + act(() => { + captured.field!.setValue('first') + }) + expect(getByTestId(container, 'title').textContent).toBe('first') + + // Second write through the SAME captured accessor. Before the fix this threw + // "Handle has been disposed". + expect(() => { + act(() => { + captured.field!.setValue('second') + }) + }).not.toThrow() + expect(getByTestId(container, 'title').textContent).toBe('second') + }) +}) diff --git a/tests/unit/handles/entityHandle.test.ts b/tests/unit/handles/entityHandle.test.ts index b972b0fb..7b92d599 100644 --- a/tests/unit/handles/entityHandle.test.ts +++ b/tests/unit/handles/entityHandle.test.ts @@ -436,22 +436,4 @@ describe('EntityHandle', () => { }) }) - - // ==================== Dispose ==================== - - describe('Dispose', () => { - test('should dispose handle and clear caches', () => { - store.setEntityData('Article', 'a-1', { id: 'a-1', title: 'Test' }, true) - const handle = createEntityHandleRaw() - - // Access some fields to populate cache - handle.field('title') - handle.hasOne('author') - - handle.dispose() - - // After dispose, handle should throw on operations - expect(() => handle.reset()).toThrow() - }) - }) })