diff --git a/openspec/changes/editor-destroy/tasks.md b/openspec/changes/editor-destroy/tasks.md index b2bf3705..545b1dce 100644 --- a/openspec/changes/editor-destroy/tasks.md +++ b/openspec/changes/editor-destroy/tasks.md @@ -1,15 +1,16 @@ ## 1. SDK contracts -- [ ] 1.1 Add optional `destroy?(): void` to `InlineTool` and declare it explicitly on `BlockTool` in `packages/sdk/src/entities/{InlineTool,BlockTool}.ts`, with type tests that tools with and without `destroy()` both satisfy their constructor types +- [x] 1.1 Add optional `destroy?(): void` to `InlineTool` and declare it explicitly on `BlockTool` in `packages/sdk/src/entities/{InlineTool,BlockTool}.ts`, with type tests that tools with and without `destroy()` both satisfy their constructor types - [ ] 1.2 Narrow `EditorJSAdapterPlugin` to a required `destroy(): void` in `packages/sdk/src/entities/EditorjsAdapterPlugin.ts`, and update the test adapter mocks in `packages/core` to implement it ## 2. Signal plumbing (AbortController rule, design D8/D8a) -- [ ] 2.1 Add an `options?: AddEventListenerOptions` argument to `EditorJSModel`'s typed `addEventListener` overloads and forward it -- [ ] 2.2 Keep the core `DocumentAPI.onUpdate` and `SelectionAPI.onCaretUpdate` tests green (`should stop calling the callback after the returned function is called`) while switching the implementation to a controller per subscription that returns `() => controller.abort()` -- [ ] 2.3 Write failing `BlockToolAdapter` tests: `should stop dispatching translated events after destroy`, and `should remove a subclass listener registered with the inherited signal on destroy`. Give the base class its own controller and a `protected readonly signal`, and abort it in `destroy()` +- [x] 2.1 Add an `options?: AddEventListenerOptions` argument to `EditorJSModel`'s typed `addEventListener` overloads and forward it +- [x] 2.2 Keep the core `DocumentAPI.onUpdate` and `SelectionAPI.onCaretUpdate` tests green (`should stop calling the callback after the returned function is called`) while switching the implementation to a controller per subscription that returns `() => controller.abort()` +- [x] 2.3 Write failing `BlockToolAdapter` tests: `should stop dispatching translated events after destroy`, and `should remove a subclass listener registered with the inherited signal on destroy`. Give the base class its own controller and a `protected readonly signal`, and abort it in `destroy()` - [ ] 2.4 Make `DOMBlockToolAdapter` pass `this.signal` to its `ui:beforeinput` registration, and remove its `destroy()` override and `#beforeInputListener` field -- [ ] 2.5 Move `UndoRedoManager`, `ClipboardPlugin`, `ShortcutsPlugin`, and `CollaborationManager` listener cleanup to one controller each. Keep their existing destroy tests green, and keep explicit cleanup only for timers and the socket +- [x] 2.5 Move `UndoRedoManager` listener cleanup to one controller, keeping explicit cleanup only for its debounce timer +- [ ] 2.6 Move `ClipboardPlugin`, `ShortcutsPlugin`, and `CollaborationManager` listener cleanup to one controller each. Keep their existing destroy tests green, and keep explicit cleanup only for timers and the socket ## 3. Selection watcher reference counting (dom-adapters) @@ -27,18 +28,18 @@ ## 5. Core services teardown -- [ ] 5.1 Write failing `BlockRenderer` tests: `should call tool destroy when its block is removed`, `should destroy every remaining tool and block adapter on destroy without dispatching BlockRemovedCoreEvent`, `should stop handling model events after destroy`, and `should not dispatch BlockAddedCoreEvent for a render that settles after destroy` -- [ ] 5.2 Track tool instances per block id in `BlockRenderer`, subscribe to the model with a controller's signal, add `destroy()`, and remove the `@todo clear block tool adapter memory` -- [ ] 5.3 Write a failing `SelectionManager` test, `should stop handling caret updates after destroy`. Implement `SelectionManager.destroy()` by aborting its controller -- [ ] 5.4 Give `InlineToolFacade` a `destroy()` that destroys its shared instance once, and `ToolsManager` a `destroy()` that calls it for every inline tool, logging a throwing hook; `Core.destroy()` calls it last (design D3, D7). Test that `SelectionManager.applyInlineTool` and `ToolsManager` validation don't destroy the shared instance +- [x] 5.1 Write failing `BlockRenderer` tests: `should call tool destroy when its block is removed`, `should destroy every remaining tool and block adapter on destroy without dispatching BlockRemovedCoreEvent`, `should stop handling model events after destroy`, and `should not dispatch BlockAddedCoreEvent for a render that settles after destroy` +- [x] 5.2 Track tool instances per block id in `BlockRenderer`, subscribe to the model with a controller's signal, add `destroy()`, and remove the `@todo clear block tool adapter memory` +- [x] 5.3 Write a failing `SelectionManager` test, `should stop handling caret updates after destroy`. Implement `SelectionManager.destroy()` by aborting its controller +- [x] 5.4 Give `InlineToolFacade` a `destroy()` that destroys its shared instance once, and `ToolsManager` a `destroy()` that calls it for every inline tool, logging a throwing hook; `Core.destroy()` calls it last (design D3, D7). Test that `SelectionManager.applyInlineTool` and `ToolsManager` validation don't destroy the shared instance ## 6. Core.destroy() -- [ ] 6.1 Write failing `Core` tests (`packages/core/src/index.spec.ts`): `should call plugin destroy in reverse construction order`, `should unregister plugin public APIs`, `should continue teardown and log when a plugin destroy throws`, `should destroy UndoRedoManager, SelectionManager, BlockRenderer and then the adapter`, `should not emit model events during destroy`, and `should be a no-op when called twice` -- [ ] 6.2 Keep plugin instances in `#initializePlugin`, track the initialization stage, and implement `destroy()` in the order from design D3 -- [ ] 6.3 Write failing tests: `should not construct anything when destroyed before initialize`, `should reject initialize with an AbortError without dispatching ready when destroyed mid-initialization`, and `should throw from use and initialize after destroy` -- [ ] 6.4 Implement the `#destroyed` checks after each `await` in `initialize()`, and the post-destroy guards in `use()` and `initialize()` -- [ ] 6.5 Write failing `ToolsManager` tests, `should not prepare the tools still queued` and `should neither create nor announce a tool whose prepare() settles after destroy`. Give `ToolsManager` an `AbortController` that `destroy()` aborts, and check its signal before and after each `prepare()` (design D4) +- [x] 6.1 Write failing `Core` tests (`packages/core/src/index.spec.ts`): `should call plugin destroy in reverse construction order`, `should unregister plugin public APIs`, `should continue teardown and log when a plugin destroy throws`, `should destroy UndoRedoManager, SelectionManager, BlockRenderer and then the adapter`, `should not emit model events during destroy`, and `should be a no-op when called twice` +- [x] 6.2 Keep plugin instances in `#initializePlugin`, track the initialization stage, and implement `destroy()` in the order from design D3 +- [x] 6.3 Write failing tests: `should not construct anything when destroyed before initialize`, `should reject initialize with an AbortError without dispatching ready when destroyed mid-initialization`, and `should throw from use and initialize after destroy` +- [x] 6.4 Implement the `#destroyed` checks after each `await` in `initialize()`, and the post-destroy guards in `use()` and `initialize()` +- [x] 6.5 Write failing `ToolsManager` tests, `should not prepare the tools still queued` and `should neither create nor announce a tool whose prepare() settles after destroy`. Give `ToolsManager` an `AbortController` that `destroy()` aborts, and check its signal before and after each `prepare()` (design D4) ## 7. UI @@ -47,7 +48,7 @@ ## 8. Bundle and integration -- [ ] 8.1 Write a failing `EditorJS` test, `should delegate destroy to Core`, and implement `EditorJS#destroy()`. Add tests that an `AbortError` from `isReady` isn't reported as an unhandled rejection while any other initialization error is logged, and handle that on the bundle's ready promise +- [x] 8.1 Write a failing `EditorJS` test, `should delegate destroy to Core`, and implement `EditorJS#destroy()`. Add tests that an `AbortError` from `isReady` isn't reported as an unhandled rejection while any other initialization error is logged, and handle that on the bundle's ready promise - [ ] 8.2 Write the integration test in `packages/editorjs`, `should leave no DOM nodes or document/holder listeners after creating and destroying several editors on one page`, spying on `document`/holder `add/removeEventListener` and checking that the counts balance and the holders are empty - [ ] 8.3 Add an integration test that destroys the editor before `isReady` settles and checks that `isReady` rejects with an `AbortError` and nothing is rendered - [ ] 8.4 Check that `CollaborationManager.destroy()` closes the `OTClient` socket when it's reached through `EditorJS#destroy()` (mock WebSocket) diff --git a/packages/core/src/api/DocumentAPI/DocumentAPI.spec.ts b/packages/core/src/api/DocumentAPI/DocumentAPI.spec.ts index c589ae66..d7019984 100644 --- a/packages/core/src/api/DocumentAPI/DocumentAPI.spec.ts +++ b/packages/core/src/api/DocumentAPI/DocumentAPI.spec.ts @@ -84,4 +84,47 @@ describe('DocumentAPI', () => { expect(dispatchEvent).toHaveBeenCalledWith(expect.objectContaining({ name: 'redo' })); }); }); + + describe('.onUpdate()', () => { + /** + * EventTarget stands in for the model, so listeners are really added and removed + * @param target - stand-in for the model + */ + const createAPI = (target: EventTarget): InstanceType => new DocumentAPI( + target as unknown as InstanceType, + {} as unknown as CoreConfigValidated, + { dispatchEvent } as unknown as EventBus + ); + + it('should call the callback on model update', () => { + const target = new EventTarget(); + const callback = jest.fn(); + + createAPI(target).onUpdate(callback); + target.dispatchEvent(new Event('update')); + + expect(callback).toHaveBeenCalledTimes(1); + }); + + it('should stop calling the callback after the returned function is called', () => { + const target = new EventTarget(); + const callback = jest.fn(); + + const unsubscribe = createAPI(target).onUpdate(callback); + + unsubscribe(); + target.dispatchEvent(new Event('update')); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('should subscribe through an abort signal', () => { + const target = new EventTarget(); + const addEventListener = jest.spyOn(target, 'addEventListener'); + + createAPI(target).onUpdate(jest.fn()); + + expect(addEventListener).toHaveBeenCalledWith('update', expect.any(Function), { signal: expect.any(AbortSignal) }); + }); + }); }); diff --git a/packages/core/src/api/DocumentAPI/DocumentAPI.ts b/packages/core/src/api/DocumentAPI/DocumentAPI.ts index e1e26b98..bb39fc1c 100644 --- a/packages/core/src/api/DocumentAPI/DocumentAPI.ts +++ b/packages/core/src/api/DocumentAPI/DocumentAPI.ts @@ -64,11 +64,11 @@ export class DocumentAPI implements DocumentApiInterface { * @param callback - callback called on model update */ public onUpdate(callback: (event: ModelEvents) => void): () => void { - this.#model.addEventListener(EventType.Changed, callback); + const controller = new AbortController(); - return () => { - this.#model.removeEventListener(EventType.Changed, callback); - }; + this.#model.addEventListener(EventType.Changed, callback, { signal: controller.signal }); + + return () => controller.abort(); } /** diff --git a/packages/core/src/api/SelectionAPI.spec.ts b/packages/core/src/api/SelectionAPI.spec.ts index cf8c5d0c..cffc34f4 100644 --- a/packages/core/src/api/SelectionAPI.spec.ts +++ b/packages/core/src/api/SelectionAPI.spec.ts @@ -50,4 +50,47 @@ describe('SelectionAPI', () => { }); }); }); + + describe('.onCaretUpdate()', () => { + /** + * EventTarget stands in for the model, so listeners are really added and removed + * @param target - stand-in for the model + */ + const createAPI = (target: EventTarget): InstanceType => new SelectionAPI( + selectionManager as unknown as InstanceType, + target as unknown as InstanceType, + {} as unknown as CoreConfigValidated + ); + + it('should call the callback on caret update', () => { + const target = new EventTarget(); + const callback = jest.fn(); + + createAPI(target).onCaretUpdate(callback); + target.dispatchEvent(new Event('update')); + + expect(callback).toHaveBeenCalledTimes(1); + }); + + it('should stop calling the callback after the returned function is called', () => { + const target = new EventTarget(); + const callback = jest.fn(); + + const unsubscribe = createAPI(target).onCaretUpdate(callback); + + unsubscribe(); + target.dispatchEvent(new Event('update')); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('should subscribe through an abort signal', () => { + const target = new EventTarget(); + const addEventListener = jest.spyOn(target, 'addEventListener'); + + createAPI(target).onCaretUpdate(jest.fn()); + + expect(addEventListener).toHaveBeenCalledWith('update', expect.any(Function), { signal: expect.any(AbortSignal) }); + }); + }); }); diff --git a/packages/core/src/api/SelectionAPI.ts b/packages/core/src/api/SelectionAPI.ts index 5e2b323a..174a4759 100644 --- a/packages/core/src/api/SelectionAPI.ts +++ b/packages/core/src/api/SelectionAPI.ts @@ -66,11 +66,11 @@ export class SelectionAPI implements SelectionApiInterface { * @param callback - callback for CaretManager updates */ public onCaretUpdate(callback: (event: CaretManagerEvents) => void): () => void { - this.#model.addEventListener(EventType.CaretManagerUpdated, callback); + const controller = new AbortController(); - return () => { - this.#model.removeEventListener(EventType.CaretManagerUpdated, callback); - }; + this.#model.addEventListener(EventType.CaretManagerUpdated, callback, { signal: controller.signal }); + + return () => controller.abort(); } /** diff --git a/packages/core/src/components/BlockRenderer.spec.ts b/packages/core/src/components/BlockRenderer.spec.ts index 4647ec1f..62bcedfd 100644 --- a/packages/core/src/components/BlockRenderer.spec.ts +++ b/packages/core/src/components/BlockRenderer.spec.ts @@ -251,3 +251,195 @@ describe('BlockRenderer (unit, mocked deps)', () => { }); }); }); + +describe('BlockRenderer lifecycle', () => { + beforeEach(() => { + (console.error as jest.Mock).mockClear(); + }); + + interface Setup { + renderer: InstanceType; + emit: (event: unknown) => Promise; + signal: () => AbortSignal | undefined; + dispatchEvent: jest.Mock; + adapter: { createBlockToolAdapter: jest.Mock; + destroyBlockToolAdapter: jest.Mock; }; + } + + /** + * Creates a renderer whose model honours the abort signal, like a real EventTarget + * @param createTool - returns the tool instance for each added block + */ + function setup(createTool: () => { render: () => Promise; + destroy?: () => void; }): Setup { + let listener: ((event: unknown) => void | Promise) | undefined; + let listenerSignal: AbortSignal | undefined; + + const model = { + addEventListener: jest.fn((_type: string, callback: (event: unknown) => void, options?: AddEventListenerOptions) => { + listener = callback; + listenerSignal = options?.signal; + }), + }; + const dispatchEvent = jest.fn(); + const adapter = { + createBlockToolAdapter: jest.fn(() => ({})), + destroyBlockToolAdapter: jest.fn(), + }; + const toolsManager = { + blockTools: { + get: jest.fn(() => ({ + name: 'tool', + create: createTool, + })), + }, + }; + + const renderer = new BlockRenderer( + model as unknown as InstanceType, + { dispatchEvent } as unknown as InstanceType, + toolsManager as unknown as InstanceType, + adapter as unknown as EditorJSAdapterPlugin + ); + + return { + renderer, + emit: async (event: unknown) => { + if (listenerSignal?.aborted === true) { + return; + } + + await listener?.(event); + }, + signal: () => listenerSignal, + dispatchEvent, + adapter, + }; + } + + const added = (id: string, blockIndex = 0): unknown => new BlockAddedEvent( + { blockIndex } as unknown as BlockIndex, + { name: 'tool', + id: id as BlockId, + data: {} }, + USER_ID + ); + + const removed = (id: string, blockIndex = 0): unknown => new BlockRemovedEvent( + { blockIndex } as unknown as BlockIndex, + { name: 'tool', + id: id as BlockId, + data: {} }, + USER_ID + ); + + it('should call tool destroy when its block is removed', async () => { + const destroy = jest.fn(); + const { emit, adapter, dispatchEvent } = setup(() => ({ render: () => Promise.resolve({}), + destroy })); + + await emit(added('a')); + await emit(removed('a')); + + expect(destroy).toHaveBeenCalledTimes(1); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('a'); + expect(dispatchEvent).toHaveBeenCalledTimes(2); + }); + + it('should destroy every remaining tool and block adapter on destroy without dispatching BlockRemovedCoreEvent', async () => { + const destroy = jest.fn(); + const { renderer, emit, adapter, dispatchEvent } = setup(() => ({ render: () => Promise.resolve({}), + destroy })); + + await emit(added('a', 0)); + await emit(added('b', 1)); + dispatchEvent.mockClear(); + + renderer.destroy(); + + expect(destroy).toHaveBeenCalledTimes(2); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('a'); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('b'); + expect(dispatchEvent).not.toHaveBeenCalled(); + }); + + it('should not destroy a removed block again on destroy', async () => { + const destroy = jest.fn(); + const { renderer, emit } = setup(() => ({ render: () => Promise.resolve({}), + destroy })); + + await emit(added('a')); + await emit(removed('a')); + renderer.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('should stop handling model events after destroy', async () => { + const { renderer, emit, signal, dispatchEvent } = setup(() => ({ render: () => Promise.resolve({}) })); + + renderer.destroy(); + await emit(added('a')); + + expect(signal()?.aborted).toBe(true); + expect(dispatchEvent).not.toHaveBeenCalled(); + }); + + it('should not dispatch BlockAddedCoreEvent for a render that settles after destroy', async () => { + let resolveRender: (value: unknown) => void = () => undefined; + const destroy = jest.fn(); + const { renderer, emit, adapter, dispatchEvent } = setup(() => ({ + render: () => new Promise((resolve) => { + resolveRender = resolve; + }), + destroy, + })); + + const pending = emit(added('a')); + + renderer.destroy(); + resolveRender({}); + await pending; + + expect(dispatchEvent).not.toHaveBeenCalled(); + expect(destroy).toHaveBeenCalledTimes(1); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('a'); + }); + + it('should keep tearing down when a tool destroy throws', async () => { + let calls = 0; + const { renderer, emit, adapter } = setup(() => ({ + render: () => Promise.resolve({}), + destroy: () => { + calls++; + throw new Error('broken tool'); + }, + })); + + await emit(added('a', 0)); + await emit(added('b', 1)); + + expect(() => renderer.destroy()).not.toThrow(); + expect(calls).toBe(2); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledTimes(2); + expect(console.error).toHaveBeenCalledWith('[BlockRenderer] Block Tool failed to destroy block a', expect.any(Error)); + }); + + it('should not log when a tool has no destroy', async () => { + const { renderer, emit } = setup(() => ({ render: () => Promise.resolve({}) })); + + await emit(added('a')); + renderer.destroy(); + + expect(console.error).not.toHaveBeenCalled(); + }); + + it('should destroy the adapter of a removed block that has no tracked tool instance', async () => { + const { emit, adapter } = setup(() => ({ render: () => Promise.resolve({}) })); + + await emit(removed('unknown')); + + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('unknown'); + expect(console.error).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/components/BlockRenderer.ts b/packages/core/src/components/BlockRenderer.ts index 79a93384..9b4d4e71 100644 --- a/packages/core/src/components/BlockRenderer.ts +++ b/packages/core/src/components/BlockRenderer.ts @@ -6,6 +6,7 @@ import { EventType, ModelEvents } from '@editorjs/sdk'; +import type { BlockId, BlockTool } from '@editorjs/sdk'; import 'reflect-metadata'; import { inject, injectable } from 'inversify'; import { TOKENS } from '../tokens.js'; @@ -53,6 +54,16 @@ export class BlockRenderer { */ #adapter: EditorJSAdapterPlugin; + /** + * Tool instance of each rendered block, destroyed when the block is removed or the renderer is destroyed + */ + #blocks = new Map(); + + /** + * Releases the model listener on destroy + */ + readonly #controller = new AbortController(); + /** * BlockRenderer constructor. * All parameters are injected through the IoC container. @@ -73,7 +84,19 @@ export class BlockRenderer { this.#adapter = adapter; // eslint-disable-next-line @typescript-eslint/no-misused-promises -- Need to bubble the promise up in case of errors - this.#model.addEventListener(EventType.Changed, event => this.#handleModelUpdate(event)); + this.#model.addEventListener(EventType.Changed, event => this.#handleModelUpdate(event), { signal: this.#controller.signal }); + } + + /** + * Stops listening to the model and releases every rendered block: its tool instance and its adapter. + * The model is left untouched and no BlockRemovedCoreEvent is dispatched + */ + public destroy(): void { + this.#controller.abort(); + + for (const id of this.#blocks.keys()) { + this.#releaseBlock(id); + } } /** @@ -112,7 +135,8 @@ export class BlockRenderer { throw new Error(`[BlockRenderer] Block Tool ${data.name} not found`); } - const blockToolAdapter = this.#adapter.createBlockToolAdapter(createBlockId(data.id), tool.name); + const blockId = createBlockId(data.id); + const blockToolAdapter = this.#adapter.createBlockToolAdapter(blockId, tool.name); const block = tool.create({ adapter: blockToolAdapter, @@ -121,9 +145,18 @@ export class BlockRenderer { readOnly: false, }); + this.#blocks.set(blockId, block); + try { const blockElement = await block.render(); + /** + * The renderer was destroyed while the block was rendering, so the block is already released + */ + if (this.#controller.signal.aborted) { + return; + } + this.#eventBus.dispatchEvent(new BlockAddedCoreEvent({ tool: tool.name, data: data.data, @@ -149,15 +182,30 @@ export class BlockRenderer { throw new Error('[BlockRenderer] Block index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); } - this.#adapter.destroyBlockToolAdapter(createBlockId(data.id)); + this.#releaseBlock(createBlockId(data.id)); this.#eventBus.dispatchEvent(new BlockRemovedCoreEvent({ tool: data.name, index: blockIndex, })); + } + + /** + * Destroys the block's tool instance and its adapter. + * A throwing tool is logged so the rest of the teardown still runs + * @param id - id of the block to release + */ + #releaseBlock(id: BlockId): void { + const block = this.#blocks.get(id); + + this.#blocks.delete(id); + + try { + block?.destroy?.(); + } catch (error) { + console.error(`[BlockRenderer] Block Tool failed to destroy block ${id}`, error); + } - /** - * @todo clear block tool adapter memory - */ + this.#adapter.destroyBlockToolAdapter(id); } } diff --git a/packages/core/src/components/SelectionManager.spec.ts b/packages/core/src/components/SelectionManager.spec.ts index 8a475340..72e94668 100644 --- a/packages/core/src/components/SelectionManager.spec.ts +++ b/packages/core/src/components/SelectionManager.spec.ts @@ -542,4 +542,72 @@ describe('SelectionManager', () => { expect(selectionManager.selectedBlocks()).toEqual([{ id: 'b0' }]); }); }); + + describe('tool and listener teardown', () => { + /** + * Sets up a caret with one valid text segment + * @param segment - segment returned by the caret index + */ + const mockCaret = (segment: Record): void => { + jest.spyOn(model, 'getCaret') + .mockReturnValue({ + index: { getTextSegments: jest.fn(() => [segment]) }, + update: jest.fn(), + } as unknown as ReturnType); + jest.spyOn(model, 'getFragments').mockReturnValue([]); + jest.spyOn(model, 'format').mockImplementation(() => undefined); + }; + + it('should not destroy the shared inline tool instance after applying it', () => { + const destroy = jest.fn(); + const toolMock = { + getFormattingOptions: jest.fn(() => ({ action: 'format', + range: [0, 3] })), + destroy, + }; + + (toolsManager as unknown as { inlineTools: Map }).inlineTools = new Map([['bold', { create: () => toolMock }]]); + mockCaret({ blockIndex: 0, + dataKey: 'text', + textRange: [0, 3] }); + + selectionManager.applyInlineTool({ toolName: 'bold' as InlineToolName }); + + expect(destroy).not.toHaveBeenCalled(); + }); + + it('should not destroy the shared inline tool instance when applying it throws', () => { + const destroy = jest.fn(); + const toolMock = { getFormattingOptions: jest.fn(), + destroy }; + + (toolsManager as unknown as { inlineTools: Map }).inlineTools = new Map([['bold', { create: () => toolMock }]]); + mockCaret({ blockIndex: 0, + dataKey: 'text' }); + + expect(() => selectionManager.applyInlineTool({ toolName: 'bold' as InlineToolName })).toThrow(); + expect(destroy).not.toHaveBeenCalled(); + }); + + it('should stop handling caret updates after destroy', () => { + // @ts-expect-error - Mocked instance + const ownModel = new EditorJSModel(); + let signal: AbortSignal | undefined; + + ownModel.addEventListener = (_type: EventType, _callback: (e: Event) => void, options?: AddEventListenerOptions) => { + signal = options?.signal; + }; + + const manager = new SelectionManager( + { userId: 'user' } as unknown as CoreConfigValidated, + ownModel, + eventBus, + toolsManager + ); + + manager.destroy(); + + expect(signal?.aborted).toBe(true); + }); + }); }); diff --git a/packages/core/src/components/SelectionManager.ts b/packages/core/src/components/SelectionManager.ts index b128ebf2..eb3dda40 100644 --- a/packages/core/src/components/SelectionManager.ts +++ b/packages/core/src/components/SelectionManager.ts @@ -47,6 +47,11 @@ export class SelectionManager { */ #toolsManager: ToolsManager; + /** + * Releases the caret listener on destroy + */ + readonly #controller = new AbortController(); + /** * @param config - Editor's config * @param model - editor model instance @@ -64,7 +69,18 @@ export class SelectionManager { this.#eventBus = eventBus; this.#toolsManager = toolsManager; - this.#model.addEventListener(EventType.CaretManagerUpdated, (event: CaretManagerEvents) => this.#handleCaretManagerUpdate(event)); + this.#model.addEventListener( + EventType.CaretManagerUpdated, + (event: CaretManagerEvents) => this.#handleCaretManagerUpdate(event), + { signal: this.#controller.signal } + ); + } + + /** + * Stops listening to caret updates + */ + public destroy(): void { + this.#controller.abort(); } /** diff --git a/packages/core/src/components/UndoRedoManager.spec.ts b/packages/core/src/components/UndoRedoManager.spec.ts index a1893e4a..bfd10a79 100644 --- a/packages/core/src/components/UndoRedoManager.spec.ts +++ b/packages/core/src/components/UndoRedoManager.spec.ts @@ -163,42 +163,44 @@ describe('UndoRedoManager', () => { it('should register a listener for model Changed events', () => { expect(model.addEventListener).toHaveBeenCalledWith( EventType.Changed, - expect.any(Function) + expect.any(Function), + { signal: expect.any(AbortSignal) } ); }); it('should register an undo listener on the eventBus', () => { expect(eventBus.addEventListener).toHaveBeenCalledWith( 'core:undo', - expect.any(Function) + expect.any(Function), + { signal: expect.any(AbortSignal) } ); }); it('should register a redo listener on the eventBus', () => { expect(eventBus.addEventListener).toHaveBeenCalledWith( 'core:redo', - expect.any(Function) + expect.any(Function), + { signal: expect.any(AbortSignal) } ); }); }); describe('.destroy()', () => { - it('should remove the undo listener from eventBus', () => { - manager.destroy(); - - expect(eventBus.removeEventListener).toHaveBeenCalledWith( - 'core:undo', - expect.any(Function) - ); - }); + /** + * Collects the signals passed to every listener registration + * @param mock - mocked addEventListener + */ + const signalsOf = (mock: jest.Mock): (AbortSignal | undefined)[] => mock.mock.calls.map( + call => (call[2] as AddEventListenerOptions | undefined)?.signal + ); - it('should remove the redo listener from eventBus', () => { + it('should abort the signal of the undo and redo listeners', () => { manager.destroy(); - expect(eventBus.removeEventListener).toHaveBeenCalledWith( - 'core:redo', - expect.any(Function) - ); + const signals = signalsOf(eventBus.addEventListener); + + expect(signals).toHaveLength(2); + expect(signals.every(signal => signal?.aborted === true)).toBe(true); }); it('should clear the debounce timer', () => { @@ -211,13 +213,12 @@ describe('UndoRedoManager', () => { expect(clearTimeoutSpy).toHaveBeenCalled(); }); - it('should remove the model updates listener', () => { + it('should abort the signal of the model updates listener', () => { manager.destroy(); - expect(model.removeEventListener).toHaveBeenCalledWith( - 'model:changed', - expect.any(Function) - ); + const [signal] = signalsOf(model.addEventListener); + + expect(signal?.aborted).toBe(true); }); }); diff --git a/packages/core/src/components/UndoRedoManager.ts b/packages/core/src/components/UndoRedoManager.ts index 3bdf018d..8abb1e1d 100644 --- a/packages/core/src/components/UndoRedoManager.ts +++ b/packages/core/src/components/UndoRedoManager.ts @@ -41,11 +41,6 @@ export class UndoRedoManager { */ #model: EditorJSModel; - /** - * Editor's EventBus instance - */ - #eventBus: EventBus; - /** * Editor configuration (provides the current user id). */ @@ -79,36 +74,9 @@ export class UndoRedoManager { #isApplying = false; /** - * Undo Core Event listener. Stored to be removed on destroy - * @param event - undo core event + * Releases the manager's model and EventBus listeners on destroy */ - #undoListener = (event: UndoCoreEvent): void => { - if (event.defaultPrevented) { - return; - } - - this.undo(); - }; - - /** - * Redo Core Event listener. Stored to be removed on destroy - * @param event - redo core event - */ - #redoListener = (event: RedoCoreEvent): void => { - if (event.defaultPrevented) { - return; - } - - this.redo(); - }; - - /** - * Model updates listener. Stored to be removed on destroy - * @param e - model event - */ - #modelUpdatesListener = (e: ModelEvents): void => { - this.#handleEvent(e); - }; + readonly #controller = new AbortController(); /** * UndoRedoManager constructor. @@ -124,12 +92,26 @@ export class UndoRedoManager { ) { this.#config = config; this.#model = model; - this.#eventBus = eventBus; - model.addEventListener(EventType.Changed, this.#modelUpdatesListener); + const { signal } = this.#controller; + + model.addEventListener(EventType.Changed, (e: ModelEvents) => this.#handleEvent(e), { signal }); + + eventBus.addEventListener(`core:${CoreEventType.Undo}`, (event: UndoCoreEvent) => { + if (event.defaultPrevented) { + return; + } + + this.undo(); + }, { signal }); + + eventBus.addEventListener(`core:${CoreEventType.Redo}`, (event: RedoCoreEvent) => { + if (event.defaultPrevented) { + return; + } - eventBus.addEventListener(`core:${CoreEventType.Undo}`, this.#undoListener); - eventBus.addEventListener(`core:${CoreEventType.Redo}`, this.#redoListener); + this.redo(); + }, { signal }); } /** @@ -180,13 +162,11 @@ export class UndoRedoManager { } /** - * Releases resources held by the manager (the pending debounce timer). + * Releases resources held by the manager: its listeners and the pending debounce timer */ public destroy(): void { clearTimeout(this.#debounceTimer); - this.#eventBus.removeEventListener(`core:${CoreEventType.Undo}`, this.#undoListener); - this.#eventBus.removeEventListener(`core:${CoreEventType.Redo}`, this.#redoListener); - this.#model.removeEventListener(EventType.Changed, this.#modelUpdatesListener); + this.#controller.abort(); } /** diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index 3338540a..4fd4c8f3 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -1,8 +1,12 @@ /* eslint-disable jsdoc/require-jsdoc -- inline test stubs */ -import { describe, it, expect } from '@jest/globals'; -import { PluginType, ToolType } from '@editorjs/sdk'; -import type { CoreConfig } from '@editorjs/sdk'; +import { describe, it, expect, jest, beforeEach } from '@jest/globals'; +import { CoreEventType, PluginType, ToolType } from '@editorjs/sdk'; +import type { CoreConfig, EditorjsPluginParams } from '@editorjs/sdk'; import Core from './index.js'; +import { UndoRedoManager } from './components/UndoRedoManager.js'; +import { SelectionManager } from './components/SelectionManager.js'; +import { BlockRenderer } from './components/BlockRenderer.js'; +import ToolsManager from './tools/ToolsManager.js'; /** * `Core` is headless: it registers nothing by default, so `initialize()` validates @@ -107,4 +111,382 @@ describe('Core', () => { expect(StubPlugin.instances).toBe(1); }); }); + + describe('destroy()', () => { + /** + * Records teardown calls across plugins, tools and the adapter, in order + */ + let log: string[]; + + beforeEach(() => { + log = []; + }); + + /** + * Adapter recording its teardown + */ + const createAdapter = (): unknown => class RecordingAdapter { + public static type = PluginType.Adapter as const; + public static name = 'recording-adapter'; + + public createBlockToolAdapter(): object { + return {}; + } + + /** + * The v2 data import assigns block ids, so the id isn't recorded + */ + public destroyBlockToolAdapter(): void { + log.push('adapter:block'); + } + + public destroy(): void { + log.push('adapter'); + } + }; + + /** + * Block tool recording its construction and teardown + */ + const createBlockTool = (): unknown => class RecordingBlockTool { + public static type = ToolType.Block as const; + public static name = 'paragraph'; + + public render(): Promise { + return Promise.resolve({}); + } + + public destroy(): void { + log.push('tool'); + } + }; + + /** + * Plugin recording its teardown under the given name + * @param name - static name the plugin registers its public API under + * @param onCreate - receives the plugin's constructor params + */ + const createPlugin = (name: string, onCreate?: (params: EditorjsPluginParams) => void): unknown => class RecordingPlugin { + public static type = PluginType.Plugin as const; + public static name = name; + + public publicApi = { name }; + + constructor(params: EditorjsPluginParams) { + onCreate?.(params); + } + + public destroy(): void { + log.push(`plugin:${name}`); + } + }; + + /** + * Creates a core with one block of data and the recording adapter and block tool registered + * @param overrides - config overrides + */ + const createCore = (overrides: Partial = {}): Core => { + const core = new Core(createConfig({ + data: { blocks: [{ id: 'block-1', + type: 'paragraph', + data: {} }] }, + ...overrides, + } as Partial)); + + core.use(createAdapter() as typeof StubAdapter); + core.use(createBlockTool() as typeof StubBlockTool); + + return core; + }; + + /** + * Lets the block render promise settle + */ + const flush = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + + it('should call plugin destroy in reverse construction order', async () => { + const core = createCore(); + + core.use(createPlugin('a') as typeof StubPlugin); + core.use(createPlugin('b') as typeof StubPlugin); + core.use(createPlugin('c') as typeof StubPlugin); + await core.initialize(); + + core.destroy(); + + expect(log.filter(entry => entry.startsWith('plugin:'))).toEqual(['plugin:c', 'plugin:b', 'plugin:a']); + }); + + it('should unregister plugin public APIs', async () => { + let params: EditorjsPluginParams | undefined; + const core = createCore(); + + core.use(createPlugin('probe', (p) => { + params = p; + }) as typeof StubPlugin); + await core.initialize(); + + expect((params?.api.plugins as Record).probe).toBeDefined(); + + core.destroy(); + + expect((params?.api.plugins as Record).probe).toBeUndefined(); + }); + + it('should continue teardown and log when a plugin destroy throws', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = createCore(); + + core.use(createPlugin('a') as typeof StubPlugin); + core.use(class BrokenPlugin { + public static type = PluginType.Plugin as const; + public static name = 'broken'; + + public destroy(): void { + throw new Error('broken plugin'); + } + } as unknown as typeof StubPlugin); + await core.initialize(); + await flush(); + + expect(() => core.destroy()).not.toThrow(); + expect(error).toHaveBeenCalledWith('[Core] Failed to destroy plugin "broken"', expect.any(Error)); + expect(log).toEqual(['plugin:a', 'tool', 'adapter:block', 'adapter']); + + error.mockRestore(); + }); + + it('should destroy plugins, then rendered blocks, then the adapter', async () => { + const core = createCore(); + + core.use(createPlugin('a') as typeof StubPlugin); + await core.initialize(); + await flush(); + + core.destroy(); + + expect(log).toEqual(['plugin:a', 'tool', 'adapter:block', 'adapter']); + }); + + it('should destroy each shared inline tool instance once, after the adapter', async () => { + const core = createCore(); + + core.use(class SharedInlineTool { + public static type = ToolType.Inline as const; + public static name = 'shared'; + + /** + * Gives the tool a validation pass, which creates its shared instance + */ + public static prepare(): Promise { + return Promise.resolve(); + } + + public destroy(): void { + log.push('inline'); + } + } as unknown as typeof StubBlockTool); + await core.initialize(); + await flush(); + + core.destroy(); + core.destroy(); + + expect(log).toEqual(['tool', 'adapter:block', 'adapter', 'inline']); + }); + + it('should not emit model events during destroy', async () => { + const onModelUpdate = jest.fn(); + const core = createCore({ onModelUpdate }); + + await core.initialize(); + await flush(); + onModelUpdate.mockClear(); + + core.destroy(); + + expect(onModelUpdate).not.toHaveBeenCalled(); + }); + + it('should stop calling onModelUpdate after destroy', async () => { + let model: { removeBlock: (userId: string, index: number) => void } | undefined; + const onModelUpdate = jest.fn((updated: unknown) => { + model = updated as typeof model; + }); + const core = createCore({ onModelUpdate }); + + await core.initialize(); + await flush(); + core.destroy(); + onModelUpdate.mockClear(); + + // An integrator that kept the model it was handed changes it after teardown + model?.removeBlock('user', 0); + + expect(model).toBeDefined(); + expect(onModelUpdate).not.toHaveBeenCalled(); + }); + + it('should be a no-op when called twice', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = createCore(); + + core.use(createPlugin('a') as typeof StubPlugin); + await core.initialize(); + await flush(); + + core.destroy(); + core.destroy(); + + expect(log).toEqual(['plugin:a', 'tool', 'adapter:block', 'adapter']); + expect(error).not.toHaveBeenCalled(); + + error.mockRestore(); + }); + + it.each([ + ['UndoRedoManager', UndoRedoManager], + ['SelectionManager', SelectionManager], + ['BlockRenderer', BlockRenderer], + ['ToolsManager', ToolsManager], + ])('should destroy %s', async (_label, Service) => { + const destroy = jest.spyOn(Service.prototype, 'destroy'); + const core = createCore(); + + await core.initialize(); + core.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); + + destroy.mockRestore(); + }); + + it.each([ + ['UndoRedoManager', UndoRedoManager], + ['SelectionManager', SelectionManager], + ['BlockRenderer', BlockRenderer], + ['ToolsManager', ToolsManager], + ])('should log and continue when %s fails to destroy', async (label, Service) => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const destroy = jest.spyOn(Service.prototype, 'destroy').mockImplementation(() => { + throw new Error('broken service'); + }); + const core = createCore(); + + await core.initialize(); + core.destroy(); + + expect(error).toHaveBeenCalledWith(`[Core] Failed to destroy ${label}`, expect.any(Error)); + expect(log).toContain('adapter'); + + destroy.mockRestore(); + error.mockRestore(); + }); + + it('should log and continue when the adapter fails to destroy', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = new Core(createConfig()); + + core.use(class BrokenAdapter { + public static type = PluginType.Adapter as const; + + public destroy(): void { + throw new Error('broken adapter'); + } + } as unknown as typeof StubAdapter); + core.use(StubBlockTool); + await core.initialize(); + + core.destroy(); + + expect(error).toHaveBeenCalledWith('[Core] Failed to destroy adapter', expect.any(Error)); + + error.mockRestore(); + }); + + it('should not log when the adapter has no destroy', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = new Core(createConfig()); + + core.use(StubAdapter); + core.use(StubBlockTool); + await core.initialize(); + + core.destroy(); + + expect(error).not.toHaveBeenCalled(); + + error.mockRestore(); + }); + + it('should not log when destroyed before initialize', () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = createCore(); + + core.destroy(); + + expect(error).not.toHaveBeenCalled(); + + error.mockRestore(); + }); + + it('should not construct anything when destroyed before initialize', () => { + const created = jest.fn(); + const core = createCore(); + + core.use(createPlugin('a', created) as typeof StubPlugin); + + core.destroy(); + + expect(created).not.toHaveBeenCalled(); + expect(log).toEqual([]); + }); + + it('should reject initialize with an AbortError without dispatching ready when destroyed mid-initialization', async () => { + let resolvePrepare: () => void = () => undefined; + const ready = jest.fn(); + const onModelUpdate = jest.fn(); + const core = createCore({ onModelUpdate }); + + core.use(createPlugin('a', ({ eventBus }) => { + eventBus.addEventListener(`core:${CoreEventType.Ready}`, ready); + }) as typeof StubPlugin); + core.use(class SlowInlineTool { + public static type = ToolType.Inline as const; + public static name = 'slow'; + + public static prepare(): Promise { + return new Promise((resolve) => { + resolvePrepare = resolve; + }); + } + } as unknown as typeof StubBlockTool); + + const initialization = core.initialize(); + + /** + * Let the tools queue reach the pending prepare() + */ + await flush(); + core.destroy(); + resolvePrepare(); + + await expect(initialization).rejects.toMatchObject({ name: 'AbortError', + message: 'Editor was destroyed during initialization' }); + await flush(); + + expect(ready).not.toHaveBeenCalled(); + expect(onModelUpdate).not.toHaveBeenCalled(); + expect(log).toEqual(['plugin:a', 'adapter']); + }); + + it('should throw from use and initialize after destroy', async () => { + const core = createCore(); + + core.destroy(); + + expect(() => core.use(createPlugin('a') as typeof StubPlugin)).toThrow(/destroyed/); + await expect(core.initialize()).rejects.toThrow(/destroyed/); + }); + }); }); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 01ab27ae..22b344ca 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -12,7 +12,9 @@ import { PluginType, ToolType, type ToolStaticOptions, - type PluginId + type PluginId, + type EditorjsPlugin, + type EditorJSAdapterPlugin } from '@editorjs/sdk'; import { composeDataFromVersion2 } from './utils/composeDataFromVersion2.js'; import ToolsManager from './tools/ToolsManager.js'; @@ -31,6 +33,51 @@ import { PluginRegistry } from './components/PluginRegistry.js'; */ const DEFAULT_HOLDER_ID = 'editorjs'; +/** + * Plugin instance created by Core, with the name its public API is registered under + */ +interface PluginInstance { + /** + * Plugin's static name + */ + name: PluginId; + + /** + * Plugin instance + */ + instance: EditorjsPlugin; +} + +/** + * Core services that hold listeners or resources and need teardown + */ +interface CoreServices { + /** + * Tracks the caret and applies inline tools + */ + selectionManager?: SelectionManager; + + /** + * Renders blocks and owns their tool instances + */ + blockRenderer?: BlockRenderer; + + /** + * Local undo/redo history + */ + undoRedoManager?: UndoRedoManager; + + /** + * Rendering adapter plugin + */ + adapter?: EditorJSAdapterPlugin; + + /** + * Owns the tool facades, and through them the shared inline tool instances + */ + toolsManager?: ToolsManager; +} + /** * Editor entry point * - initializes Model @@ -59,6 +106,27 @@ export default class Core { */ #plugins: Container; + /** + * Plugin instances in construction order, destroyed in reverse order + */ + #pluginInstances: PluginInstance[] = []; + + /** + * Core services resolved during initialization, destroyed by {@link destroy}. + * Kept here so teardown never resolves a service that initialization didn't reach + */ + #services: CoreServices = {}; + + /** + * Releases Core's own listeners on destroy + */ + readonly #controller = new AbortController(); + + /** + * Set by {@link destroy}. A destroyed core can't be used or initialized again + */ + #destroyed = false; + /** * @param config - Editor configuration */ @@ -101,7 +169,7 @@ export default class Core { if (config.onModelUpdate !== undefined) { this.#model.addEventListener(EventType.Changed, () => { config.onModelUpdate?.(this.#model); - }); + }, { signal: this.#controller.signal }); } } @@ -129,6 +197,8 @@ export default class Core { pluginOrTool: ToolConstructable | EditorjsPluginConstructor | EditorjsAdapterPluginConstructor, options?: ToolStaticOptions ): Core { + this.#assertNotDestroyed(); + const pluginType = pluginOrTool.type; switch (pluginType) { @@ -155,6 +225,7 @@ export default class Core { * Initializes the core */ public async initialize(): Promise { + this.#assertNotDestroyed(); this.#validatePreconditions(); const { blocks } = composeDataFromVersion2(this.#config.data ?? { blocks: [] }); @@ -167,18 +238,27 @@ export default class Core { * @todo add e2e initialization tests * Currently only BlockRenderer would be enough, but that would be hard to debug. Easier just add every module here */ - this.#iocContainer.get(SelectionManager); + this.#services.selectionManager = this.#iocContainer.get(SelectionManager); this.#iocContainer.get(BlocksManager); - this.#iocContainer.get(BlockRenderer); + this.#services.blockRenderer = this.#iocContainer.get(BlockRenderer); + this.#services.adapter = this.#iocContainer.get(TOKENS.Adapter); + this.#services.toolsManager = this.#iocContainer.get(ToolsManager); this.#initializePlugins(); await this.#initializeTools(); + /** + * destroy() was called while tools were being prepared: it already tore down what exists + */ + if (this.#destroyed) { + throw new DOMException('Editor was destroyed during initialization', 'AbortError'); + } + /** * UndoRedoManager should go after plugins as it uses defaultPrevented on undo/redo events which can be set by plugins * @todo Figure out how to make initialization less complex */ - this.#iocContainer.get(UndoRedoManager); + this.#services.undoRedoManager = this.#iocContainer.get(UndoRedoManager); this.#model.initializeDocument({ blocks }); @@ -187,6 +267,64 @@ export default class Core { eventBus.dispatchEvent(new CoreEventBase(CoreEventType.Ready, undefined)); } + /** + * Tears the editor down: plugins in reverse construction order, core services, rendered blocks, and the adapter. + * The document model is left untouched, so no removal reaches undo history or collaboration. + * Safe to call more than once and while {@link initialize} is pending. After it, `use()` and `initialize()` throw + */ + public destroy(): void { + /** + * Plugin instances and services are cleared below, so a second call finds nothing left to destroy + */ + this.#destroyed = true; + this.#controller.abort(); + + const registry = this.#iocContainer.get(PluginRegistry); + + for (const { name, instance } of this.#pluginInstances.reverse()) { + this.#safely(`plugin "${name}"`, () => instance.destroy?.()); + registry.unregister(name); + } + + this.#pluginInstances = []; + + const { undoRedoManager, selectionManager, blockRenderer, adapter, toolsManager } = this.#services; + + this.#safely('UndoRedoManager', () => undoRedoManager?.destroy()); + this.#safely('SelectionManager', () => selectionManager?.destroy()); + this.#safely('BlockRenderer', () => blockRenderer?.destroy()); + this.#safely('adapter', () => adapter?.destroy?.()); + + /** + * Last: the shared inline tool instances are used by the adapter and the UI until they're gone + */ + this.#safely('ToolsManager', () => toolsManager?.destroy()); + + this.#services = {}; + } + + /** + * Runs one teardown step, logging its error so the remaining steps still run + * @param label - what is being destroyed, for the log + * @param step - teardown step + */ + #safely(label: string, step: () => void): void { + try { + step(); + } catch (error) { + console.error(`[Core] Failed to destroy ${label}`, error); + } + } + + /** + * Throws if {@link destroy} has been called + */ + #assertNotDestroyed(): void { + if (this.#destroyed) { + throw new Error('Editor has been destroyed. Create a new instance instead'); + } + } + /** * Validates that the caller has registered everything the headless engine needs before booting. * Throws with a clear message instead of failing deep inside module resolution. @@ -252,6 +390,9 @@ export default class Core { eventBus, }); + this.#pluginInstances.push({ name: plugin.name, + instance }); + if (instance.publicApi !== undefined) { this.#iocContainer.get(PluginRegistry).register(plugin.name, instance.publicApi); } diff --git a/packages/core/src/tools/ToolsManager.spec.ts b/packages/core/src/tools/ToolsManager.spec.ts new file mode 100644 index 00000000..17ed2116 --- /dev/null +++ b/packages/core/src/tools/ToolsManager.spec.ts @@ -0,0 +1,288 @@ +import { describe, expect, it, jest } from '@jest/globals'; +import type { CoreConfigValidated, InlineToolConstructor } from '@editorjs/sdk'; +import { EventBus, ToolType } from '@editorjs/sdk'; +import ToolsManager from './ToolsManager.js'; +import type { EditorAPI } from '../api/index.js'; + +/** + * Inline tool whose static prepare() settles only when the test calls finish() + */ +interface PendingTool { + /** + * The tool class + */ + tool: InlineToolConstructor; + + /** + * Its static prepare() + */ + prepare: jest.Mock; + + /** + * Settles the pending prepare() + */ + finish: () => void; +} + +describe('ToolsManager', () => { + describe('.prepareTools()', () => { + it('should not destroy the shared inline tool instance it validates', async () => { + const destroy = jest.fn(); + + /** + * Inline tool with a static prepare(), so the validation branch runs + */ + class ProbeInlineTool { + public static type = ToolType.Inline; + + public static name = 'probe'; + + public static prepare = jest.fn(() => Promise.resolve()); + + public destroy = destroy; + } + + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + new EventBus() + ); + + await manager.prepareTools([[ProbeInlineTool as unknown as InlineToolConstructor, undefined]]); + + expect(destroy).not.toHaveBeenCalled(); + }); + + /** + * Prepares a single inline tool whose instance is built by `create` + * @param instance - prototype members of the tool instance + */ + const prepareInlineTool = async (instance: Record): Promise => { + /** + * Inline tool whose instance carries the given members + */ + class ProbeInlineTool { + public static type = ToolType.Inline; + + public static name = 'probe'; + + /** + * Copies the given members onto the instance + */ + constructor() { + Object.assign(this, instance); + } + + /** + * Static prepare, so the validation branch runs + */ + public static prepare(): Promise { + return Promise.resolve(); + } + } + + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + new EventBus() + ); + + await manager.prepareTools([[ProbeInlineTool as unknown as InlineToolConstructor, undefined]]); + + return manager; + }; + + it('should make an inline tool without render available', async () => { + const manager = await prepareInlineTool({}); + + expect(manager.inlineTools.get('probe')).toBeDefined(); + expect(manager.unavailable.get('probe')).toBeUndefined(); + }); + + it('should mark an inline tool implementing the v2 render method as unavailable', async () => { + const log = jest.spyOn(console, 'log').mockImplementation(() => undefined); + const manager = await prepareInlineTool({ render: () => undefined }); + + expect(manager.inlineTools.get('probe')).toBeUndefined(); + expect(manager.unavailable.get('probe')).toBeDefined(); + + log.mockRestore(); + }); + }); + + describe('.prepareTools() interrupted by .destroy()', () => { + /** + * Inline tool class with a static prepare() that settles when the test says so + * @param name - static name the tool is registered under + * @param constructed - called by the tool's constructor + */ + const pendingTool = (name: string, constructed: () => void = jest.fn()): PendingTool => { + let finish: () => void = () => undefined; + const prepare = jest.fn(() => new Promise((resolve) => { + finish = resolve; + })); + + /** + * Inline tool whose preparation the test controls + */ + class PendingInlineTool { + public static type = ToolType.Inline; + + public static name = name; + + public static prepare = prepare; + + /** + * Reports its construction + */ + constructor() { + constructed(); + } + } + + return { + tool: PendingInlineTool as unknown as InlineToolConstructor, + prepare, + finish: () => finish(), + }; + }; + + /** + * Lets queued promise jobs run + */ + const flush = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + + it('should not prepare the tools still queued', async () => { + const first = pendingTool('first'); + const second = pendingTool('second'); + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + new EventBus() + ); + + const preparing = manager.prepareTools([[first.tool, undefined], [second.tool, undefined]]); + + await flush(); + manager.destroy(); + first.finish(); + await preparing; + + expect(first.prepare).toHaveBeenCalledTimes(1); + expect(second.prepare).not.toHaveBeenCalled(); + }); + + it('should neither create nor announce a tool whose prepare() settles after destroy', async () => { + const constructed = jest.fn(); + const pending = pendingTool('late', constructed); + const eventBus = new EventBus(); + const dispatch = jest.spyOn(eventBus, 'dispatchEvent'); + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + eventBus + ); + + const preparing = manager.prepareTools([[pending.tool, undefined]]); + + await flush(); + manager.destroy(); + pending.finish(); + await preparing; + + expect(pending.prepare).toHaveBeenCalledTimes(1); + expect(constructed).not.toHaveBeenCalled(); + expect(dispatch).not.toHaveBeenCalled(); + expect(manager.available.get('late')).toBeUndefined(); + expect(manager.unavailable.get('late')).toBeUndefined(); + }); + }); + + describe('.destroy()', () => { + /** + * Inline tool class whose instances call `onDestroy` from their destroy() hook + * @param name - static name the tool is registered under + * @param onDestroy - called by the instance's destroy() + * @param withPrepare - give the tool a static prepare(), so prepareTools validates it + */ + const inlineTool = (name: string, onDestroy: () => void, withPrepare = false): InlineToolConstructor => { + /** + * Inline tool that reports when its instance is destroyed + */ + class ProbeInlineTool { + public static type = ToolType.Inline; + + public static name = name; + + /** + * Destroy hook under test + */ + public destroy(): void { + onDestroy(); + } + } + + if (withPrepare) { + Object.assign(ProbeInlineTool, { prepare: (): Promise => Promise.resolve() }); + } + + return ProbeInlineTool as unknown as InlineToolConstructor; + }; + + /** + * Creates a manager with the given tools prepared + * @param tools - inline tool classes to prepare + */ + const prepared = async (...tools: InlineToolConstructor[]): Promise => { + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + new EventBus() + ); + + await manager.prepareTools(tools.map(tool => [tool, undefined])); + + return manager; + }; + + it('should destroy each inline tool instance once', async () => { + const destroyA = jest.fn(); + const destroyB = jest.fn(); + const manager = await prepared(inlineTool('a', destroyA), inlineTool('b', destroyB, true)); + + manager.inlineTools.get('a')?.create(); + + manager.destroy(); + manager.destroy(); + + expect(destroyA).toHaveBeenCalledTimes(1); + expect(destroyB).toHaveBeenCalledTimes(1); + }); + + it('should not create an instance for a tool that was never used', async () => { + const destroy = jest.fn(); + const manager = await prepared(inlineTool('unused', destroy)); + + manager.destroy(); + + expect(destroy).not.toHaveBeenCalled(); + }); + + it('should log a throwing destroy() and still destroy the other tools', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const destroyB = jest.fn(); + const manager = await prepared( + inlineTool('a', () => { + throw new Error('broken tool'); + }, true), + inlineTool('b', destroyB, true) + ); + + expect(() => manager.destroy()).not.toThrow(); + expect(destroyB).toHaveBeenCalledTimes(1); + expect(error).toHaveBeenCalledWith('[ToolsManager] Inline tool a failed to destroy', expect.any(Error)); + + error.mockRestore(); + }); + }); +}); diff --git a/packages/core/src/tools/ToolsManager.ts b/packages/core/src/tools/ToolsManager.ts index 297f30fd..af2244d7 100644 --- a/packages/core/src/tools/ToolsManager.ts +++ b/packages/core/src/tools/ToolsManager.ts @@ -56,6 +56,12 @@ export default class ToolsManager { */ #unavailableTools = new ToolsCollection(); + /** + * Aborted by {@link destroy}: tools still queued for preparation are skipped, and a tool whose + * prepare() settles afterwards is neither created nor registered + */ + readonly #controller = new AbortController(); + /** * Returns available Tools */ @@ -110,12 +116,40 @@ export default class ToolsManager { this.#validateTools(); } + /** + * Destroys the shared inline tool instances, once each. + * + * Every inline tool facade hands the same instance to all its consumers (formatting, selection, + * the inline toolbar), so none of them destroys it: the facade owns it and is destroyed here, on + * editor teardown. A throwing hook is logged and the remaining tools are still destroyed. + * + * Also stops a {@link prepareTools} still running: no tool is prepared, created or announced after this + */ + public destroy(): void { + this.#controller.abort(); + + const facades = new Set([...this.#availableTools.values(), ...this.#unavailableTools.values()]); + + for (const facade of facades) { + if (!facade.isInline()) { + continue; + } + + try { + facade.destroy(); + } catch (error) { + console.error(`[ToolsManager] Inline tool ${facade.name} failed to destroy`, error); + } + } + } + /** * Calls tools prepare method if it exists and adds tools to relevant collection (available or unavailable tools) * @param tools - tools to prepare and their settings */ public async prepareTools(tools: [ToolConstructable, ToolStaticOptions | undefined][]): Promise { const promiseQueue = new PromiseQueue(); + const { signal } = this.#controller; const setToAvailableToolsCollection = (toolName: string, tool: ToolFacadeClass): void => { this.#availableTools.set(toolName, tool); @@ -134,6 +168,13 @@ export default class ToolsManager { if (isFunction(toolConstructor.prepare)) { void promiseQueue.add(async () => { + /** + * The editor was destroyed while earlier tools were being prepared + */ + if (signal.aborted) { + return; + } + try { const tool = factory.get(toolName); @@ -145,12 +186,24 @@ export default class ToolsManager { config: tool.config, }); + /** + * The editor was destroyed while this tool was preparing. An inline instance created now + * would outlive destroy(), and ToolLoaded would reach a torn-down editor + */ + if (signal.aborted) { + return; + } + if (tool.isInline()) { /** * Some Tools validation */ const inlineToolRequiredMethods = ['render']; - const notImplementedMethods = inlineToolRequiredMethods.filter(method => tool.create()[method as keyof InlineTool] !== undefined); + /** + * The facade's shared instance: it stays alive for the other consumers and is destroyed on teardown + */ + const instance = tool.create(); + const notImplementedMethods = inlineToolRequiredMethods.filter(method => instance[method as keyof InlineTool] !== undefined); if (notImplementedMethods.length > 0) { /** diff --git a/packages/editorjs/src/index.spec.ts b/packages/editorjs/src/index.spec.ts index 5e3166e4..201c6bec 100644 --- a/packages/editorjs/src/index.spec.ts +++ b/packages/editorjs/src/index.spec.ts @@ -9,6 +9,7 @@ import { jest, describe, it, expect, beforeEach } from '@jest/globals'; */ const use = jest.fn(); const initialize = jest.fn<() => Promise>(); +const destroy = jest.fn(); /** * Minimal Core stand-in recording `use()` calls and delegating `initialize()`. @@ -16,6 +17,7 @@ const initialize = jest.fn<() => Promise>(); class MockCore { public use = use; public initialize = initialize; + public destroy = destroy; } jest.unstable_mockModule('@editorjs/core', () => ({ default: MockCore })); @@ -40,6 +42,40 @@ describe('EditorJS bundle', () => { beforeEach(() => { use.mockClear(); initialize.mockReset(); + destroy.mockClear(); + }); + + describe('isReady interrupted by destroy', () => { + /** + * Lets an unhandled rejection surface: Jest fails the test that leaves one behind + */ + const flush = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + + it('should not leave an AbortError from isReady unhandled', async () => { + initialize.mockRejectedValue(new DOMException('Editor was destroyed during initialization', 'AbortError')); + + void new EditorJS({} as any); + + await expect(flush()).resolves.toBeUndefined(); + }); + + it('should still reject isReady with the AbortError', async () => { + initialize.mockRejectedValue(new DOMException('Editor was destroyed during initialization', 'AbortError')); + + const editor = new EditorJS({} as any); + + await expect(editor.isReady).rejects.toMatchObject({ name: 'AbortError' }); + }); + }); + + it('should delegate destroy to Core', () => { + initialize.mockResolvedValue(undefined); + + const editor = new EditorJS({} as any); + + editor.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); }); it('exposes isReady that resolves when Core initialization completes', async () => { @@ -52,11 +88,16 @@ describe('EditorJS bundle', () => { }); it('rejects isReady when Core initialization fails', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + initialize.mockRejectedValue(new Error('init failed')); const editor = new EditorJS({} as any); await expect(editor.isReady).rejects.toThrow('init failed'); + expect(error).toHaveBeenCalledWith('[EditorJS] Initialization failed', expect.any(Error)); + + error.mockRestore(); }); it('registers a rendering adapter on the underlying Core', () => { diff --git a/packages/editorjs/src/index.ts b/packages/editorjs/src/index.ts index 67281ee2..8f382485 100644 --- a/packages/editorjs/src/index.ts +++ b/packages/editorjs/src/index.ts @@ -11,6 +11,7 @@ import { ClipboardPlugin } from '@editorjs/clipboard-plugin'; import { ShortcutsPlugin } from '@editorjs/shortcuts-plugin'; import { EditorjsUI, BlocksUI, InlineToolbarUI, ToolbarUI, ToolboxUI } from '@editorjs/ui'; import { mergeTools } from './mergeTools.js'; +import { reportUnlessAborted } from './reportUnlessAborted.js'; /** * Default tools registered by the bundle, keyed later by their static `name`. @@ -94,14 +95,28 @@ export default class EditorJS { this.#core.use(ToolboxUI); this.#readyPromise = this.#core.initialize(); + + /** + * `isReady` may be left unawaited: ignore the AbortError a destroy before ready causes, log anything else + */ + reportUnlessAborted(this.#readyPromise); } /** - * Resolves once the editor has finished initializing; rejects if initialization fails. + * Resolves once the editor has finished initializing. Rejects if initialization fails, + * or with an `AbortError` if the editor is destroyed before it is ready */ public get isReady(): Promise { return this.#readyPromise; } + + /** + * Tears the editor down: removes its DOM, closes the collaboration connection, and releases plugins and tools. + * Safe to call more than once and before {@link isReady} settles + */ + public destroy(): void { + this.#core.destroy(); + } } export { Core }; diff --git a/packages/editorjs/src/reportUnlessAborted.spec.ts b/packages/editorjs/src/reportUnlessAborted.spec.ts new file mode 100644 index 00000000..9f23566e --- /dev/null +++ b/packages/editorjs/src/reportUnlessAborted.spec.ts @@ -0,0 +1,51 @@ +import { afterEach, beforeEach, describe, expect, it, jest } from '@jest/globals'; +import { reportUnlessAborted } from './reportUnlessAborted.js'; + +/** + * Lets the attached handler run + */ +const flush = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + +describe('reportUnlessAborted', () => { + let error: jest.SpiedFunction; + + beforeEach(() => { + error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + }); + + afterEach(() => { + error.mockRestore(); + }); + + it('should ignore an AbortError rejection', async () => { + reportUnlessAborted(Promise.reject(new DOMException('aborted', 'AbortError'))); + await flush(); + + expect(error).not.toHaveBeenCalled(); + }); + + it('should log any other rejection', async () => { + const failure = new Error('init failed'); + + reportUnlessAborted(Promise.reject(failure)); + await flush(); + + expect(error).toHaveBeenCalledWith('[EditorJS] Initialization failed', failure); + }); + + it('should log a DOMException of another name', async () => { + const failure = new DOMException('bad state', 'InvalidStateError'); + + reportUnlessAborted(Promise.reject(failure)); + await flush(); + + expect(error).toHaveBeenCalledWith('[EditorJS] Initialization failed', failure); + }); + + it('should not log when the promise resolves', async () => { + reportUnlessAborted(Promise.resolve()); + await flush(); + + expect(error).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/editorjs/src/reportUnlessAborted.ts b/packages/editorjs/src/reportUnlessAborted.ts new file mode 100644 index 00000000..b568db7b --- /dev/null +++ b/packages/editorjs/src/reportUnlessAborted.ts @@ -0,0 +1,17 @@ +/** + * Handles the bundle's own ready promise so its rejection is never reported as unhandled. + * + * Destroying the editor before it is ready rejects `isReady` with an `AbortError`: that is expected + * and ignored. Any other initialization error is logged, so it isn't lost when nobody awaits `isReady`. + * Callers awaiting `isReady` still get the rejection, since this only attaches a handler. + * @param promise - the ready promise + */ +export function reportUnlessAborted(promise: Promise): void { + promise.catch((error: unknown) => { + if (error instanceof DOMException && error.name === 'AbortError') { + return; + } + + console.error('[EditorJS] Initialization failed', error); + }); +} diff --git a/packages/model/src/EditorJSModel.ts b/packages/model/src/EditorJSModel.ts index 8b9d7521..3682e415 100644 --- a/packages/model/src/EditorJSModel.ts +++ b/packages/model/src/EditorJSModel.ts @@ -36,12 +36,12 @@ export interface EditorJSModel { /** * Overload for EditorDocument events */ - addEventListener(type: EventType.Changed, listener: (event: K) => void): void; + addEventListener(type: EventType.Changed, listener: (event: K) => void, options?: AddEventListenerOptions | boolean): void; /** * Overload for CaretManager events */ - addEventListener(type: EventType.CaretManagerUpdated, listener: (event: K) => void): void; + addEventListener(type: EventType.CaretManagerUpdated, listener: (event: K) => void, options?: AddEventListenerOptions | boolean): void; } /** diff --git a/packages/sdk/src/entities/BlockTool.ts b/packages/sdk/src/entities/BlockTool.ts index 1f77f69c..a696f97c 100644 --- a/packages/sdk/src/entities/BlockTool.ts +++ b/packages/sdk/src/entities/BlockTool.ts @@ -163,7 +163,13 @@ export type BlockTool< */ // eslint-disable-next-line @typescript-eslint/no-unused-vars, @typescript-eslint/no-explicit-any Config extends ToolConfig = any -> = Omit; +> = Omit & { + /** + * Releases resources held by the instance. + * Called once, when the block is removed or the editor is destroyed + */ + destroy?(): void; +}; /** * Block Tool constructor class diff --git a/packages/sdk/src/entities/BlockToolAdapter.spec.ts b/packages/sdk/src/entities/BlockToolAdapter.spec.ts new file mode 100644 index 00000000..78c2465a --- /dev/null +++ b/packages/sdk/src/entities/BlockToolAdapter.spec.ts @@ -0,0 +1,114 @@ +/* eslint-disable jsdoc/require-jsdoc */ + +import { describe, expect, it, jest } from '@jest/globals'; +import type { CoreConfig } from './Config.js'; +import type { EditorAPI } from '../api/index.js'; +import type { BlockId, EventBus, ModelEvents } from '@editorjs/model-types'; +import { BlockToolAdapter } from './BlockToolAdapter.js'; + +const MODEL_EVENT = 'model:update'; +const BUS_EVENT = 'ui:probe'; + +/** + * Adapter subclass that registers its own listener with the inherited signal + */ +class ProbeAdapter extends BlockToolAdapter { + public readonly modelUpdates = jest.fn(); + + public readonly busEvents = jest.fn(); + + constructor(config: Required, api: EditorAPI, eventBus: EventBus) { + super(config, api, eventBus); + + eventBus.addEventListener(BUS_EVENT, () => this.busEvents(), { signal: this.signal }); + } + + protected handleModelUpdate(event: ModelEvents): void { + this.modelUpdates(event); + } +} + +/** + * Adapter under test with the targets standing in for the model and the EventBus + */ +interface Harness { + adapter: ProbeAdapter; + model: EventTarget; + eventBus: EventTarget; +} + +/** + * Creates an adapter wired to real EventTargets standing in for the model and the EventBus + */ +function createAdapter(): Harness { + const model = new EventTarget(); + const eventBus = new EventTarget(); + const blockId = 'block' as BlockId; + + const api = { + document: { + onUpdate: (callback: EventListener) => { + const controller = new AbortController(); + + model.addEventListener(MODEL_EVENT, callback, { signal: controller.signal }); + + return () => controller.abort(); + }, + }, + blocks: { + getIdByIndex: () => blockId, + }, + } as unknown as EditorAPI; + + const adapter = new ProbeAdapter({} as Required, api, eventBus as EventBus); + + adapter.setBlockId(blockId); + + return { adapter, + model, + eventBus }; +} + +/** + * Model event shaped enough for the adapter's index check + */ +function modelEvent(): Event { + return Object.assign(new Event(MODEL_EVENT), { detail: { index: { blockIndex: 0 } } }); +} + +describe('BlockToolAdapter', () => { + it('should handle model updates before destroy', () => { + const { adapter, model } = createAdapter(); + + model.dispatchEvent(modelEvent()); + + expect(adapter.modelUpdates).toHaveBeenCalledTimes(1); + }); + + it('should stop handling model updates after destroy', () => { + const { adapter, model } = createAdapter(); + + adapter.destroy(); + model.dispatchEvent(modelEvent()); + + expect(adapter.modelUpdates).not.toHaveBeenCalled(); + }); + + it('should remove a subclass listener registered with the inherited signal on destroy', () => { + const { adapter, eventBus } = createAdapter(); + + eventBus.dispatchEvent(new Event(BUS_EVENT)); + adapter.destroy(); + eventBus.dispatchEvent(new Event(BUS_EVENT)); + + expect(adapter.busEvents).toHaveBeenCalledTimes(1); + }); + + it('should abort the inherited signal on destroy', () => { + const { adapter } = createAdapter(); + + adapter.destroy(); + + expect((adapter as unknown as { signal: AbortSignal }).signal.aborted).toBe(true); + }); +}); diff --git a/packages/sdk/src/entities/BlockToolAdapter.ts b/packages/sdk/src/entities/BlockToolAdapter.ts index faa6d212..0d95045f 100644 --- a/packages/sdk/src/entities/BlockToolAdapter.ts +++ b/packages/sdk/src/entities/BlockToolAdapter.ts @@ -38,9 +38,15 @@ export abstract class BlockToolAdapter extends EventTarget { protected eventBus: EventBus; /** - * Stored reference to the model change listener so it can be removed on destroy. + * Releases every listener registered by the adapter and its subclasses on destroy */ - #modelChangeListenerCleanup: () => void; + readonly #controller = new AbortController(); + + /** + * Signal subclasses pass to their own listener registrations. + * It is aborted by {@link destroy}, so subclasses don't need to remove listeners themselves + */ + protected readonly signal: AbortSignal = this.#controller.signal; /** * @param config - editor's configuration @@ -54,19 +60,20 @@ export abstract class BlockToolAdapter extends EventTarget { this.config = config; this.eventBus = eventBus; - this.#modelChangeListenerCleanup = this.#api.document.onUpdate( + const unsubscribe = this.#api.document.onUpdate( ((event: ModelEvents) => this.#handleModelUpdate(event)) as EventListener ); + + this.signal.addEventListener('abort', unsubscribe); } /** * Releases all resources held by this adapter. - * Removes the model change listener registered in the constructor. - * Subclasses that register additional listeners should override this method, - * call `super.destroy()`, and then remove their own listeners. + * Aborts {@link signal}, removing the model listener and every listener subclasses registered with it. + * Subclasses override this method only to release resources that aren't listeners, and call `super.destroy()` */ public destroy(): void { - this.#modelChangeListenerCleanup(); + this.#controller.abort(); } /** diff --git a/packages/sdk/src/entities/InlineTool.ts b/packages/sdk/src/entities/InlineTool.ts index 3e167e87..da7be561 100644 --- a/packages/sdk/src/entities/InlineTool.ts +++ b/packages/sdk/src/entities/InlineTool.ts @@ -125,6 +125,12 @@ export interface InlineTool extends Omit; + + /** + * Releases resources held by the instance. + * Called once, when the editor stops using this instance + */ + destroy?(): void; } /** diff --git a/packages/sdk/src/entities/ToolDestroy.spec.ts b/packages/sdk/src/entities/ToolDestroy.spec.ts new file mode 100644 index 00000000..cade0252 --- /dev/null +++ b/packages/sdk/src/entities/ToolDestroy.spec.ts @@ -0,0 +1,56 @@ +import { describe, expect, it, jest } from '@jest/globals'; +import type { BlockTool } from './BlockTool.js'; +import type { InlineTool } from './InlineTool.js'; + +/** + * Builds the minimal inline tool members, so each case only adds what it checks + */ +function inlineToolBase(): Omit { + return { + isActive: () => false, + getFormattingOptions: () => ({ action: 'format', + range: [0, 0] }) as never, + createWrapper: () => ({}) as HTMLElement, + getToolbarConfig: () => ({}) as never, + }; +} + +describe('Tool destroy contract', () => { + it('should accept an inline tool that implements destroy', () => { + const destroy = jest.fn(); + const tool: InlineTool = { + ...inlineToolBase(), + destroy, + }; + + tool.destroy?.(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('should accept an inline tool without destroy', () => { + const tool: InlineTool = inlineToolBase(); + + expect('destroy' in tool).toBe(false); + }); + + it('should accept a block tool that implements destroy', () => { + const destroy = jest.fn(); + const tool: BlockTool = { + render: () => ({}) as HTMLElement, + destroy, + }; + + tool.destroy?.(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('should accept a block tool without destroy', () => { + const tool: BlockTool = { + render: () => ({}) as HTMLElement, + }; + + expect('destroy' in tool).toBe(false); + }); +}); diff --git a/packages/sdk/src/tools/facades/InlineToolFacade.spec.ts b/packages/sdk/src/tools/facades/InlineToolFacade.spec.ts new file mode 100644 index 00000000..2c07b347 --- /dev/null +++ b/packages/sdk/src/tools/facades/InlineToolFacade.spec.ts @@ -0,0 +1,96 @@ +/* eslint-disable jsdoc/require-jsdoc */ + +import { describe, expect, it, jest } from '@jest/globals'; +import type { InlineToolConstructor } from '../../entities/index.js'; +import { ToolType } from '../../entities/index.js'; +import type { ToolOptions } from './BaseToolFacade.js'; +import { InlineToolFacade } from './InlineToolFacade.js'; +import type { EditorAPI } from '../../api'; + +/** + * Facade under test, with a record of how many instances its tool class constructed + */ +interface Probe { + facade: InlineToolFacade; + constructed: jest.Mock; +} + +/** + * Creates a facade around an inline tool class whose instances record their construction and destruction + * @param destroy - the instances' destroy() hook, or undefined for a tool without one + */ +function createFacade(destroy?: () => void): Probe { + const constructed = jest.fn(); + + class MockInlineTool { + public static type = ToolType.Inline; + + public destroy = destroy; + + constructor() { + constructed(); + } + } + + const facade = new InlineToolFacade({ + api: {} as EditorAPI, + constructable: MockInlineTool as unknown as InlineToolConstructor, + isDefault: false, + name: 'probe', + useToolOptions: {} as ToolOptions, + }); + + return { facade, + constructed }; +} + +describe('InlineToolFacade', () => { + it('should return the same instance from every create()', () => { + const { facade, constructed } = createFacade(); + + expect(facade.create()).toBe(facade.create()); + expect(constructed).toHaveBeenCalledTimes(1); + }); + + it('should call the instance\'s destroy() once on destroy', () => { + const destroy = jest.fn(); + const { facade } = createFacade(destroy); + + facade.create(); + facade.destroy(); + facade.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('should not construct an instance on destroy when none was created', () => { + const { facade, constructed } = createFacade(jest.fn()); + + facade.destroy(); + + expect(constructed).not.toHaveBeenCalled(); + }); + + it('should support a tool without destroy()', () => { + const { facade } = createFacade(); + + facade.create(); + + expect(() => facade.destroy()).not.toThrow(); + }); + + it('should drop the instance even when its destroy() throws', () => { + const destroy = jest.fn(() => { + throw new Error('broken tool'); + }); + const { facade } = createFacade(destroy); + + facade.create(); + + expect(() => facade.destroy()).toThrow('broken tool'); + + facade.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/sdk/src/tools/facades/InlineToolFacade.ts b/packages/sdk/src/tools/facades/InlineToolFacade.ts index a1faeab4..0d57b4a0 100644 --- a/packages/sdk/src/tools/facades/InlineToolFacade.ts +++ b/packages/sdk/src/tools/facades/InlineToolFacade.ts @@ -38,7 +38,8 @@ export class InlineToolFacade extends BaseToolFacade