From 52fc5cc7124c6fe6a2ffa5f1a81f52472fa47563 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 6 Oct 2026 00:48:28 +0100 Subject: [PATCH 1/6] feat(core): add editor destroy lifecycle - Core#destroy(): destroys plugins in reverse construction order and unregisters their public APIs, then UndoRedoManager, SelectionManager, BlockRenderer and the adapter. Idempotent, safe before and during initialize(), never mutates the model. use()/initialize() throw after it. - EditorJS#destroy() delegates to Core. - BlockRenderer keeps each block's tool instance and destroys it on block removal and on teardown; a render settling after teardown is dropped. - Optional destroy() on BlockTool/InlineTool; throwaway inline tool instances (SelectionManager, ToolsManager validation) are destroyed right after use. - Listeners are released through AbortController: onUpdate/onCaretUpdate subscribe through their own controller, BlockToolAdapter owns one and exposes a protected signal to subclasses, core services abort theirs on destroy. Model addEventListener overloads accept options. Part of the editor-destroy OpenSpec change. Co-Authored-By: Claude Opus 5.5 --- openspec/changes/editor-destroy/tasks.md | 25 +- .../src/api/DocumentAPI/DocumentAPI.spec.ts | 43 +++ .../core/src/api/DocumentAPI/DocumentAPI.ts | 8 +- packages/core/src/api/SelectionAPI.spec.ts | 43 +++ packages/core/src/api/SelectionAPI.ts | 8 +- .../core/src/components/BlockRenderer.spec.ts | 169 ++++++++++++ packages/core/src/components/BlockRenderer.ts | 60 ++++- .../src/components/SelectionManager.spec.ts | 68 +++++ .../core/src/components/SelectionManager.ts | 101 ++++--- .../src/components/UndoRedoManager.spec.ts | 43 +-- .../core/src/components/UndoRedoManager.ts | 64 ++--- packages/core/src/index.spec.ts | 247 +++++++++++++++++- packages/core/src/index.ts | 133 +++++++++- packages/core/src/tools/ToolsManager.spec.ts | 36 +++ packages/core/src/tools/ToolsManager.ts | 8 +- packages/editorjs/src/index.spec.ts | 13 + packages/editorjs/src/index.ts | 8 + packages/model/src/EditorJSModel.ts | 4 +- packages/sdk/src/entities/BlockTool.ts | 8 +- .../sdk/src/entities/BlockToolAdapter.spec.ts | 114 ++++++++ packages/sdk/src/entities/BlockToolAdapter.ts | 21 +- packages/sdk/src/entities/InlineTool.ts | 6 + packages/sdk/src/entities/ToolDestroy.spec.ts | 56 ++++ 23 files changed, 1140 insertions(+), 146 deletions(-) create mode 100644 packages/core/src/tools/ToolsManager.spec.ts create mode 100644 packages/sdk/src/entities/BlockToolAdapter.spec.ts create mode 100644 packages/sdk/src/entities/ToolDestroy.spec.ts diff --git a/openspec/changes/editor-destroy/tasks.md b/openspec/changes/editor-destroy/tasks.md index 1b333d40..7a56bd0b 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,17 +28,17 @@ ## 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 Write a failing test, `should destroy the throwaway inline tool instance after applying it`, for `SelectionManager.applyInlineTool`, and do the same for the instance created during `ToolsManager` validation. Call `destroy?.()` in a `finally` +- [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 Write a failing test, `should destroy the throwaway inline tool instance after applying it`, for `SelectionManager.applyInlineTool`, and do the same for the instance created during `ToolsManager` validation. Call `destroy?.()` in a `finally` ## 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 +- [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 - [ ] 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()` +- [x] 6.4 Implement the `#destroyed` checks after each `await` in `initialize()`, and the post-destroy guards in `use()` and `initialize()` ## 7. UI teardown 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..b3e312e7 100644 --- a/packages/core/src/components/BlockRenderer.spec.ts +++ b/packages/core/src/components/BlockRenderer.spec.ts @@ -251,3 +251,172 @@ describe('BlockRenderer (unit, mocked deps)', () => { }); }); }); + +describe('BlockRenderer lifecycle', () => { + 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); + }); +}); 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..082c9fa1 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 destroy the throwaway 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).toHaveBeenCalledTimes(1); + }); + + it('should destroy the throwaway 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).toHaveBeenCalledTimes(1); + }); + + 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..ea70f17a 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(); } /** @@ -175,56 +191,63 @@ export class SelectionManager { throw new Error(`SelectionManager[applyInlineTool]: tool ${toolName} is not attached`); } - for (const segment of segments) { - const textRange = segment.textRange; - const blockIndex = segment.blockIndex; - const dataKey = segment.dataKey; - - if (textRange === undefined) { - throw new IndexError('TextRange of the index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); - } + /** + * The instance is only used for this call, so it is destroyed right after + */ + try { + for (const segment of segments) { + const textRange = segment.textRange; + const blockIndex = segment.blockIndex; + const dataKey = segment.dataKey; + + if (textRange === undefined) { + throw new IndexError('TextRange of the index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); + } - if (blockIndex === undefined) { - throw new IndexError('BlockIndex should be defined. Probably something wrong with the Editor Model. Please, report this issue'); - } + if (blockIndex === undefined) { + throw new IndexError('BlockIndex should be defined. Probably something wrong with the Editor Model. Please, report this issue'); + } - if (dataKey === undefined) { - throw new IndexError('DataKey of the index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); - } + if (dataKey === undefined) { + throw new IndexError('DataKey of the index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); + } - const fragments = this.#model.getFragments(blockIndex, dataKey, ...textRange, toolName); + const fragments = this.#model.getFragments(blockIndex, dataKey, ...textRange, toolName); - const { action, range } = tool.getFormattingOptions(textRange, fragments); + const { action, range } = tool.getFormattingOptions(textRange, fragments); - switch (actionOverride ?? action) { - case FormattingAction.Format: - this.#model.format(userId, blockIndex, dataKey, toolName, ...range, createInlineToolData(data)); + switch (actionOverride ?? action) { + case FormattingAction.Format: + this.#model.format(userId, blockIndex, dataKey, toolName, ...range, createInlineToolData(data)); - break; - case FormattingAction.Unformat: - this.#model.unformat(userId, blockIndex, dataKey, toolName, ...range); + break; + case FormattingAction.Unformat: + this.#model.unformat(userId, blockIndex, dataKey, toolName, ...range); - break; - } + break; + } - /** - * Keep selection param is applied only for the current user - */ - if (userId === this.#config.userId) { - if (keepSelection) { - caret?.update(caretIndex); - } else { - // For composite selections, don't try to add textRange since composite indices - // must not have root-level textRange. Only set textRange for single-segment selections. - const selectedSegments = (caretIndex as TextIndex).getTextSegments(); - - if (selectedSegments.length === 1 && selectedSegments[0].textRange !== undefined) { - caret?.update(caretIndex.withTextRange([selectedSegments[0].textRange[1], selectedSegments[0].textRange[1]])); - } else { + /** + * Keep selection param is applied only for the current user + */ + if (userId === this.#config.userId) { + if (keepSelection) { caret?.update(caretIndex); + } else { + // For composite selections, don't try to add textRange since composite indices + // must not have root-level textRange. Only set textRange for single-segment selections. + const selectedSegments = (caretIndex as TextIndex).getTextSegments(); + + if (selectedSegments.length === 1 && selectedSegments[0].textRange !== undefined) { + caret?.update(caretIndex.withTextRange([selectedSegments[0].textRange[1], selectedSegments[0].textRange[1]])); + } else { + caret?.update(caretIndex); + } } } } + } finally { + tool.destroy?.(); } }; 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..a81fe4d8 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -1,7 +1,7 @@ /* 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'; /** @@ -107,4 +107,245 @@ 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).toHaveBeenCalled(); + 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 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 be a no-op when called twice', async () => { + 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']); + }); + + 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 resolve initialize 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).resolves.toBeUndefined(); + 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..5a97c5bb 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,46 @@ 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; +} + /** * Editor entry point * - initializes Model @@ -59,6 +101,22 @@ 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 = {}; + + /** + * Set by {@link destroy}. A destroyed core can't be used or initialized again + */ + #destroyed = false; + /** * @param config - Editor configuration */ @@ -129,6 +187,8 @@ export default class Core { pluginOrTool: ToolConstructable | EditorjsPluginConstructor | EditorjsAdapterPluginConstructor, options?: ToolStaticOptions ): Core { + this.#assertNotDestroyed(); + const pluginType = pluginOrTool.type; switch (pluginType) { @@ -155,6 +215,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 +228,26 @@ 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.#initializePlugins(); await this.#initializeTools(); + /** + * destroy() was called while tools were being prepared: it already tore down what exists + */ + if (this.#destroyed) { + return; + } + /** * 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 +256,59 @@ 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 { + if (this.#destroyed) { + return; + } + + this.#destroyed = true; + + 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 } = this.#services; + + this.#safely('UndoRedoManager', () => undoRedoManager?.destroy()); + this.#safely('SelectionManager', () => selectionManager?.destroy()); + this.#safely('BlockRenderer', () => blockRenderer?.destroy()); + this.#safely('adapter', () => adapter?.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 +374,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..dabb0f3b --- /dev/null +++ b/packages/core/src/tools/ToolsManager.spec.ts @@ -0,0 +1,36 @@ +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'; + +describe('ToolsManager', () => { + describe('.prepareTools()', () => { + it('should destroy the inline tool instance created for validation', 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).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/packages/core/src/tools/ToolsManager.ts b/packages/core/src/tools/ToolsManager.ts index 297f30fd..42792d38 100644 --- a/packages/core/src/tools/ToolsManager.ts +++ b/packages/core/src/tools/ToolsManager.ts @@ -150,7 +150,13 @@ export default class ToolsManager { * Some Tools validation */ const inlineToolRequiredMethods = ['render']; - const notImplementedMethods = inlineToolRequiredMethods.filter(method => tool.create()[method as keyof InlineTool] !== undefined); + const instance = tool.create(); + const notImplementedMethods = inlineToolRequiredMethods.filter(method => instance[method as keyof InlineTool] !== undefined); + + /** + * The instance is only used for validation + */ + instance.destroy?.(); if (notImplementedMethods.length > 0) { /** diff --git a/packages/editorjs/src/index.spec.ts b/packages/editorjs/src/index.spec.ts index fa64ca57..4cfc0355 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,17 @@ describe('EditorJS bundle', () => { beforeEach(() => { use.mockClear(); initialize.mockReset(); + destroy.mockClear(); + }); + + 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 () => { diff --git a/packages/editorjs/src/index.ts b/packages/editorjs/src/index.ts index 2a42d0e6..4412ba38 100644 --- a/packages/editorjs/src/index.ts +++ b/packages/editorjs/src/index.ts @@ -99,6 +99,14 @@ export default class EditorJS { 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/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..8b828f90 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, { once: true }); } /** * 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); + }); +}); From d397f4dd70e8a3662a1619ef4db44c152275313f Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 6 Oct 2026 01:16:38 +0100 Subject: [PATCH 2/6] test(core): kill surviving mutants in the destroy lifecycle - Cover teardown error logging, services that were never resolved, an adapter without destroy(), and blocks removed without a tracked tool. - Cover ToolsManager inline tool validation around the destroyed validation instance. - Drop the redundant destroyed guard in Core.destroy(): clearing plugin instances and services already makes a second call a no-op. - Drop { once: true } on the BlockToolAdapter abort listener: a signal aborts only once. Co-Authored-By: Claude Opus 5.5 --- .../core/src/components/BlockRenderer.spec.ts | 23 +++++ packages/core/src/index.spec.ts | 93 ++++++++++++++++++- packages/core/src/index.ts | 7 +- packages/core/src/tools/ToolsManager.spec.ts | 56 +++++++++++ packages/sdk/src/entities/BlockToolAdapter.ts | 2 +- 5 files changed, 175 insertions(+), 6 deletions(-) diff --git a/packages/core/src/components/BlockRenderer.spec.ts b/packages/core/src/components/BlockRenderer.spec.ts index b3e312e7..62bcedfd 100644 --- a/packages/core/src/components/BlockRenderer.spec.ts +++ b/packages/core/src/components/BlockRenderer.spec.ts @@ -253,6 +253,10 @@ describe('BlockRenderer (unit, mocked deps)', () => { }); describe('BlockRenderer lifecycle', () => { + beforeEach(() => { + (console.error as jest.Mock).mockClear(); + }); + interface Setup { renderer: InstanceType; emit: (event: unknown) => Promise; @@ -418,5 +422,24 @@ describe('BlockRenderer lifecycle', () => { 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/index.spec.ts b/packages/core/src/index.spec.ts index a81fe4d8..06b38c0e 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -3,6 +3,9 @@ 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'; /** * `Core` is headless: it registers nothing by default, so `initialize()` validates @@ -246,7 +249,7 @@ describe('Core', () => { await flush(); expect(() => core.destroy()).not.toThrow(); - expect(error).toHaveBeenCalled(); + expect(error).toHaveBeenCalledWith('[Core] Failed to destroy plugin "broken"', expect.any(Error)); expect(log).toEqual(['plugin:a', 'tool', 'adapter:block', 'adapter']); error.mockRestore(); @@ -278,6 +281,7 @@ describe('Core', () => { }); 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); @@ -288,6 +292,93 @@ describe('Core', () => { 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], + ])('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], + ])('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', () => { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5a97c5bb..e29fa08c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -262,10 +262,9 @@ export default class Core { * Safe to call more than once and while {@link initialize} is pending. After it, `use()` and `initialize()` throw */ public destroy(): void { - if (this.#destroyed) { - return; - } - + /** + * Plugin instances and services are cleared below, so a second call finds nothing left to destroy + */ this.#destroyed = true; const registry = this.#iocContainer.get(PluginRegistry); diff --git a/packages/core/src/tools/ToolsManager.spec.ts b/packages/core/src/tools/ToolsManager.spec.ts index dabb0f3b..c727a48b 100644 --- a/packages/core/src/tools/ToolsManager.spec.ts +++ b/packages/core/src/tools/ToolsManager.spec.ts @@ -32,5 +32,61 @@ describe('ToolsManager', () => { expect(destroy).toHaveBeenCalledTimes(1); }); + + /** + * 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(); + }); }); }); diff --git a/packages/sdk/src/entities/BlockToolAdapter.ts b/packages/sdk/src/entities/BlockToolAdapter.ts index 8b828f90..0d95045f 100644 --- a/packages/sdk/src/entities/BlockToolAdapter.ts +++ b/packages/sdk/src/entities/BlockToolAdapter.ts @@ -64,7 +64,7 @@ export abstract class BlockToolAdapter extends EventTarget { ((event: ModelEvents) => this.#handleModelUpdate(event)) as EventListener ); - this.signal.addEventListener('abort', unsubscribe, { once: true }); + this.signal.addEventListener('abort', unsubscribe); } /** From e2d0909721c59838499163c004d33ee63b24eb4f Mon Sep 17 00:00:00 2001 From: gohabereg Date: Tue, 6 Oct 2026 18:13:17 +0100 Subject: [PATCH 3/6] feat(core): reject an initialize() interrupted by destroy() with AbortError - Core.initialize() rejects with a DOMException named AbortError when destroy() runs while tools are being prepared, instead of resolving. - The EditorJS bundle handles its own ready promise: an AbortError is ignored, any other initialization error is logged, so neither is reported as an unhandled rejection. Awaiting isReady still rejects. Co-Authored-By: Claude Opus 5.5 --- openspec/changes/editor-destroy/tasks.md | 4 +- packages/core/src/index.spec.ts | 5 +- packages/core/src/index.ts | 2 +- packages/editorjs/src/index.spec.ts | 28 ++++++++++ packages/editorjs/src/index.ts | 9 +++- .../editorjs/src/reportUnlessAborted.spec.ts | 51 +++++++++++++++++++ packages/editorjs/src/reportUnlessAborted.ts | 17 +++++++ 7 files changed, 110 insertions(+), 6 deletions(-) create mode 100644 packages/editorjs/src/reportUnlessAborted.spec.ts create mode 100644 packages/editorjs/src/reportUnlessAborted.ts diff --git a/openspec/changes/editor-destroy/tasks.md b/openspec/changes/editor-destroy/tasks.md index 7a56bd0b..d6c0510c 100644 --- a/openspec/changes/editor-destroy/tasks.md +++ b/openspec/changes/editor-destroy/tasks.md @@ -37,7 +37,7 @@ - [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 -- [ ] 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.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()` ## 7. UI teardown @@ -49,7 +49,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/index.spec.ts b/packages/core/src/index.spec.ts index 06b38c0e..9016def4 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -393,7 +393,7 @@ describe('Core', () => { expect(log).toEqual([]); }); - it('should resolve initialize without dispatching ready when destroyed mid-initialization', async () => { + 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(); @@ -422,7 +422,8 @@ describe('Core', () => { core.destroy(); resolvePrepare(); - await expect(initialization).resolves.toBeUndefined(); + await expect(initialization).rejects.toMatchObject({ name: 'AbortError', + message: 'Editor was destroyed during initialization' }); await flush(); expect(ready).not.toHaveBeenCalled(); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e29fa08c..58031324 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -240,7 +240,7 @@ export default class Core { * destroy() was called while tools were being prepared: it already tore down what exists */ if (this.#destroyed) { - return; + throw new DOMException('Editor was destroyed during initialization', 'AbortError'); } /** diff --git a/packages/editorjs/src/index.spec.ts b/packages/editorjs/src/index.spec.ts index 4cfc0355..0a83ae2a 100644 --- a/packages/editorjs/src/index.spec.ts +++ b/packages/editorjs/src/index.spec.ts @@ -45,6 +45,29 @@ describe('EditorJS bundle', () => { 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); @@ -65,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 4412ba38..b4910be3 100644 --- a/packages/editorjs/src/index.ts +++ b/packages/editorjs/src/index.ts @@ -10,6 +10,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`. @@ -91,10 +92,16 @@ 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; 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); + }); +} From 2a1ea01cdc6da311b01c34a6814869013544a899 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Wed, 4 Nov 2026 18:38:51 +0000 Subject: [PATCH 4/6] fix(core): destroy each shared inline tool instance once, from its facade InlineToolFacade.create() hands the same cached instance to every consumer, but ToolsManager's validation and SelectionManager destroyed it after each use, so a tool with a destroy() hook was torn down at startup and after every formatting action, and then used again. - InlineToolFacade.destroy() destroys the shared instance once, dropping it before the hook runs. - ToolsManager.destroy() destroys every inline tool facade; a throwing hook is logged and the remaining tools are still destroyed. - Core.destroy() calls it last, after the adapter, since the adapter and the UI use the instances until then. - Validation and SelectionManager.applyInlineTool no longer destroy the instance. Co-Authored-By: Claude Opus 5.5 --- .../src/components/SelectionManager.spec.ts | 8 +- .../core/src/components/SelectionManager.ts | 83 ++++++++-------- packages/core/src/index.spec.ts | 30 ++++++ packages/core/src/index.ts | 13 ++- packages/core/src/tools/ToolsManager.spec.ts | 92 +++++++++++++++++- packages/core/src/tools/ToolsManager.ts | 31 +++++- .../tools/facades/InlineToolFacade.spec.ts | 96 +++++++++++++++++++ .../sdk/src/tools/facades/InlineToolFacade.ts | 18 +++- 8 files changed, 313 insertions(+), 58 deletions(-) create mode 100644 packages/sdk/src/tools/facades/InlineToolFacade.spec.ts diff --git a/packages/core/src/components/SelectionManager.spec.ts b/packages/core/src/components/SelectionManager.spec.ts index 082c9fa1..72e94668 100644 --- a/packages/core/src/components/SelectionManager.spec.ts +++ b/packages/core/src/components/SelectionManager.spec.ts @@ -558,7 +558,7 @@ describe('SelectionManager', () => { jest.spyOn(model, 'format').mockImplementation(() => undefined); }; - it('should destroy the throwaway inline tool instance after applying it', () => { + it('should not destroy the shared inline tool instance after applying it', () => { const destroy = jest.fn(); const toolMock = { getFormattingOptions: jest.fn(() => ({ action: 'format', @@ -573,10 +573,10 @@ describe('SelectionManager', () => { selectionManager.applyInlineTool({ toolName: 'bold' as InlineToolName }); - expect(destroy).toHaveBeenCalledTimes(1); + expect(destroy).not.toHaveBeenCalled(); }); - it('should destroy the throwaway inline tool instance when applying it throws', () => { + it('should not destroy the shared inline tool instance when applying it throws', () => { const destroy = jest.fn(); const toolMock = { getFormattingOptions: jest.fn(), destroy }; @@ -586,7 +586,7 @@ describe('SelectionManager', () => { dataKey: 'text' }); expect(() => selectionManager.applyInlineTool({ toolName: 'bold' as InlineToolName })).toThrow(); - expect(destroy).toHaveBeenCalledTimes(1); + expect(destroy).not.toHaveBeenCalled(); }); it('should stop handling caret updates after destroy', () => { diff --git a/packages/core/src/components/SelectionManager.ts b/packages/core/src/components/SelectionManager.ts index ea70f17a..eb3dda40 100644 --- a/packages/core/src/components/SelectionManager.ts +++ b/packages/core/src/components/SelectionManager.ts @@ -191,63 +191,56 @@ export class SelectionManager { throw new Error(`SelectionManager[applyInlineTool]: tool ${toolName} is not attached`); } - /** - * The instance is only used for this call, so it is destroyed right after - */ - try { - for (const segment of segments) { - const textRange = segment.textRange; - const blockIndex = segment.blockIndex; - const dataKey = segment.dataKey; - - if (textRange === undefined) { - throw new IndexError('TextRange of the index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); - } + for (const segment of segments) { + const textRange = segment.textRange; + const blockIndex = segment.blockIndex; + const dataKey = segment.dataKey; - if (blockIndex === undefined) { - throw new IndexError('BlockIndex should be defined. Probably something wrong with the Editor Model. Please, report this issue'); - } + if (textRange === undefined) { + throw new IndexError('TextRange of the index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); + } - if (dataKey === undefined) { - throw new IndexError('DataKey of the index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); - } + if (blockIndex === undefined) { + throw new IndexError('BlockIndex should be defined. Probably something wrong with the Editor Model. Please, report this issue'); + } - const fragments = this.#model.getFragments(blockIndex, dataKey, ...textRange, toolName); + if (dataKey === undefined) { + throw new IndexError('DataKey of the index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); + } - const { action, range } = tool.getFormattingOptions(textRange, fragments); + const fragments = this.#model.getFragments(blockIndex, dataKey, ...textRange, toolName); - switch (actionOverride ?? action) { - case FormattingAction.Format: - this.#model.format(userId, blockIndex, dataKey, toolName, ...range, createInlineToolData(data)); + const { action, range } = tool.getFormattingOptions(textRange, fragments); - break; - case FormattingAction.Unformat: - this.#model.unformat(userId, blockIndex, dataKey, toolName, ...range); + switch (actionOverride ?? action) { + case FormattingAction.Format: + this.#model.format(userId, blockIndex, dataKey, toolName, ...range, createInlineToolData(data)); - break; - } + break; + case FormattingAction.Unformat: + this.#model.unformat(userId, blockIndex, dataKey, toolName, ...range); - /** - * Keep selection param is applied only for the current user - */ - if (userId === this.#config.userId) { - if (keepSelection) { - caret?.update(caretIndex); + break; + } + + /** + * Keep selection param is applied only for the current user + */ + if (userId === this.#config.userId) { + if (keepSelection) { + caret?.update(caretIndex); + } else { + // For composite selections, don't try to add textRange since composite indices + // must not have root-level textRange. Only set textRange for single-segment selections. + const selectedSegments = (caretIndex as TextIndex).getTextSegments(); + + if (selectedSegments.length === 1 && selectedSegments[0].textRange !== undefined) { + caret?.update(caretIndex.withTextRange([selectedSegments[0].textRange[1], selectedSegments[0].textRange[1]])); } else { - // For composite selections, don't try to add textRange since composite indices - // must not have root-level textRange. Only set textRange for single-segment selections. - const selectedSegments = (caretIndex as TextIndex).getTextSegments(); - - if (selectedSegments.length === 1 && selectedSegments[0].textRange !== undefined) { - caret?.update(caretIndex.withTextRange([selectedSegments[0].textRange[1], selectedSegments[0].textRange[1]])); - } else { - caret?.update(caretIndex); - } + caret?.update(caretIndex); } } } - } finally { - tool.destroy?.(); } }; diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index 9016def4..0de8ac42 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -6,6 +6,7 @@ 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 @@ -267,6 +268,33 @@ describe('Core', () => { 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 }); @@ -301,6 +329,7 @@ describe('Core', () => { ['UndoRedoManager', UndoRedoManager], ['SelectionManager', SelectionManager], ['BlockRenderer', BlockRenderer], + ['ToolsManager', ToolsManager], ])('should destroy %s', async (_label, Service) => { const destroy = jest.spyOn(Service.prototype, 'destroy'); const core = createCore(); @@ -317,6 +346,7 @@ describe('Core', () => { ['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(() => { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 58031324..fb0a02d2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -71,6 +71,11 @@ interface CoreServices { * Rendering adapter plugin */ adapter?: EditorJSAdapterPlugin; + + /** + * Owns the tool facades, and through them the shared inline tool instances + */ + toolsManager?: ToolsManager; } /** @@ -232,6 +237,7 @@ export default class Core { this.#iocContainer.get(BlocksManager); 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(); @@ -276,13 +282,18 @@ export default class Core { this.#pluginInstances = []; - const { undoRedoManager, selectionManager, blockRenderer, adapter } = this.#services; + 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 = {}; } diff --git a/packages/core/src/tools/ToolsManager.spec.ts b/packages/core/src/tools/ToolsManager.spec.ts index c727a48b..1148da0c 100644 --- a/packages/core/src/tools/ToolsManager.spec.ts +++ b/packages/core/src/tools/ToolsManager.spec.ts @@ -6,7 +6,7 @@ import type { EditorAPI } from '../api/index.js'; describe('ToolsManager', () => { describe('.prepareTools()', () => { - it('should destroy the inline tool instance created for validation', async () => { + it('should not destroy the shared inline tool instance it validates', async () => { const destroy = jest.fn(); /** @@ -30,7 +30,7 @@ describe('ToolsManager', () => { await manager.prepareTools([[ProbeInlineTool as unknown as InlineToolConstructor, undefined]]); - expect(destroy).toHaveBeenCalledTimes(1); + expect(destroy).not.toHaveBeenCalled(); }); /** @@ -89,4 +89,92 @@ describe('ToolsManager', () => { log.mockRestore(); }); }); + + 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 42792d38..dff6e51f 100644 --- a/packages/core/src/tools/ToolsManager.ts +++ b/packages/core/src/tools/ToolsManager.ts @@ -110,6 +110,29 @@ 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 + */ + public destroy(): void { + 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 @@ -150,13 +173,11 @@ export default class ToolsManager { * Some Tools validation */ const inlineToolRequiredMethods = ['render']; - const instance = tool.create(); - const notImplementedMethods = inlineToolRequiredMethods.filter(method => instance[method as keyof InlineTool] !== undefined); - /** - * The instance is only used for validation + * The facade's shared instance: it stays alive for the other consumers and is destroyed on teardown */ - instance.destroy?.(); + const instance = tool.create(); + const notImplementedMethods = inlineToolRequiredMethods.filter(method => instance[method as keyof InlineTool] !== undefined); if (notImplementedMethods.length > 0) { /** 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 Date: Wed, 4 Nov 2026 22:32:13 +0000 Subject: [PATCH 5/6] fix(core): release the onModelUpdate listener on destroy Core registered its onModelUpdate model listener without a way to remove it. An integrator that kept the model it was handed could still reach the callback after teardown. The listener now uses Core's own controller, which destroy() aborts. Co-Authored-By: Claude Opus 5.5 --- packages/core/src/index.spec.ts | 19 +++++++++++++++++++ packages/core/src/index.ts | 8 +++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index 0de8ac42..4fd4c8f3 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -308,6 +308,25 @@ describe('Core', () => { 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(); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index fb0a02d2..22b344ca 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -117,6 +117,11 @@ export default class Core { */ #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 */ @@ -164,7 +169,7 @@ export default class Core { if (config.onModelUpdate !== undefined) { this.#model.addEventListener(EventType.Changed, () => { config.onModelUpdate?.(this.#model); - }); + }, { signal: this.#controller.signal }); } } @@ -272,6 +277,7 @@ export default class Core { * 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); From 3c3ddcefed7d223cba05195a90bd6ef951d31782 Mon Sep 17 00:00:00 2001 From: gohabereg Date: Wed, 4 Nov 2026 23:59:57 +0000 Subject: [PATCH 6/6] fix(core): stop preparing tools once the editor is destroyed The tool preparation queue kept running after destroy(). Tools still queued were prepared, and a tool whose prepare() settled after teardown was created, registered and announced with ToolLoaded. Its inline instance then outlived ToolsManager.destroy(). ToolsManager now owns an AbortController that destroy() aborts. Each queued job checks the signal before calling prepare() and again once it settles. A prepare() already running can't be interrupted, since it takes no signal, so its result is dropped. Co-Authored-By: Claude Opus 5.5 --- packages/core/src/tools/ToolsManager.spec.ts | 108 +++++++++++++++++++ packages/core/src/tools/ToolsManager.ts | 28 ++++- 2 files changed, 135 insertions(+), 1 deletion(-) diff --git a/packages/core/src/tools/ToolsManager.spec.ts b/packages/core/src/tools/ToolsManager.spec.ts index 1148da0c..17ed2116 100644 --- a/packages/core/src/tools/ToolsManager.spec.ts +++ b/packages/core/src/tools/ToolsManager.spec.ts @@ -4,6 +4,26 @@ 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 () => { @@ -90,6 +110,94 @@ describe('ToolsManager', () => { }); }); + 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 diff --git a/packages/core/src/tools/ToolsManager.ts b/packages/core/src/tools/ToolsManager.ts index dff6e51f..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 */ @@ -115,9 +121,13 @@ export default class ToolsManager { * * 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 + * 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) { @@ -139,6 +149,7 @@ export default class ToolsManager { */ 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); @@ -157,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); @@ -168,6 +186,14 @@ 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