diff --git a/openspec/changes/editor-destroy/.openspec.yaml b/openspec/changes/editor-destroy/.openspec.yaml new file mode 100644 index 00000000..e3966d7a --- /dev/null +++ b/openspec/changes/editor-destroy/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-10-05 diff --git a/openspec/changes/editor-destroy/design.md b/openspec/changes/editor-destroy/design.md new file mode 100644 index 00000000..8692f0f8 --- /dev/null +++ b/openspec/changes/editor-destroy/design.md @@ -0,0 +1,138 @@ +## Context + +`Core.initialize()` builds the editor in this order: adapter binding → `SelectionManager`, `BlocksManager`, `BlockRenderer` → plugins (in `use()` order) → tools → `UndoRedoManager` → `model.initializeDocument()` → `core:ready`. `#initializePlugin` creates each plugin instance and then drops the reference, keeping only its `publicApi`. That is why no teardown path exists today. + +Resources that outlive an editor if nobody releases them: + +| Owner | Resource | Today | +|---|---|---| +| `EditorjsUI` | wrapper `div` appended to the integrator's holder | covered by #174, out of scope here | +| `BlocksUI` | blocks holder DOM listeners (`beforeinput`, `keydown`, `copy`), block wrappers | covered by #174, out of scope here | +| `ToolbarUI` / `ToolboxUI` / `InlineToolbarUI` | EventBus listeners, popovers | covered by #174, out of scope here | +| `CaretAdapter` | subscription to the singleton `useSelectionChange` watcher (`document` `selectionchange`) and `api.selection.onCaretUpdate` | `@todo Unsubscribe on adapter destruction` | +| `FormattingAdapter` | `core:ToolLoaded` and `api.document.onUpdate` listeners | never released | +| `ToolsManager` | inline tool facades, each holding the one instance its consumers share | never destroyed | +| `DOMAdapters` | `DOMBlockToolAdapter` per block (model and `ui:beforeinput` listeners) | released only on block removal | +| `BlockRenderer` | model listener, block tool instances | instances aren't tracked (`@todo clear block tool adapter memory`) | +| `SelectionManager` | model `CaretManagerUpdated` listener | never released | +| `UndoRedoManager` | model and EventBus listeners, debounce timer | `destroy()` exists, never called | +| `CollaborationManager` | `OTClient` WebSocket, debounce timer, EventBus/model listeners | `destroy()` exists, never called | +| `ClipboardPlugin`, `ShortcutsPlugin` | EventBus listeners | `destroy()` exists, never called | + +The model and the EventBus belong to one `Core` instance. Once nothing outside holds a reference to them, the garbage collector reclaims them along with their listeners. The real leaks are therefore things reachable from outside the editor: `document` and holder listeners, DOM nodes under the integrator's holder, sockets, and timers. We still unsubscribe model and EventBus listeners explicitly, so a destroyed editor stops reacting to anything. That matters when an integrator keeps a reference to `api` or the model, and when an async render settles after teardown. + +## Goals / Non-Goals + +**Goals:** +- `Core#destroy()` / `EditorJS#destroy()` leave nothing behind that can be reached from outside the editor: no DOM, no `document`/holder listeners, no open socket, no pending timer. +- Every editor-owned component stops reacting to events after teardown. +- Tools get a hook (`destroy()`) to release their own resources. +- Several editors on one page can be created and destroyed independently. + +**Non-Goals:** +- Re-initializing a destroyed instance. Integrators construct a new one. +- Async teardown, such as waiting for the socket to finish closing or flushing pending collaboration ops. See Open Questions. +- Detecting listeners that third-party plugins or tools forget to remove. Each plugin and tool is responsible for its own subscriptions. +- Block Tunes teardown. Tunes aren't instantiated per block yet (see the tunes-as-plugins work). + +## Decisions + +### D1. `destroy()` is synchronous +Every existing `destroy()` in the repo is synchronous, and `EditorjsPlugin.destroy?(): void` is already the contract. `OTClient.close()` is fire-and-forget: it closes the socket once its open promise resolves. A synchronous `destroy()` also fits unmount hooks in UI frameworks, which can't await anything. +*Alternative*: `destroy(): Promise` that awaits socket close. Rejected for now because it would change the plugin contract and isn't needed to stop leaks. + +### D2. Core keeps plugin instances and tears down in reverse construction order +`#initializePlugin` pushes each instance onto a `#pluginInstances` array, paired with its `name`. `destroy()` walks the array backwards, calls `destroy?.()` inside a try/catch with `console.error`, and calls `PluginRegistry.unregister(name)`. Reverse order means a plugin that depends on another one's `publicApi` or DOM, like `EditorjsUI` hosting the elements other UI plugins render, is torn down after its dependents. + +### D3. Teardown order inside `Core.destroy()` +1. Set `#destroyed = true`, so `use()` and `initialize()` throw from now on. Plugin instances and services are cleared as they're destroyed, so a second `destroy()` finds nothing left and is a no-op. +2. Plugins, in reverse order (D2). UI disappears first, collaboration closes its socket and stops sending ops, and keyboard and clipboard plugins detach. +3. `UndoRedoManager.destroy()`, then `SelectionManager.destroy()`. +4. `BlockRenderer.destroy()`: for each tracked block, call the tool instance's `destroy?.()`, then `adapter.destroyBlockToolAdapter(id)`. Remove the model listener. No `BlockRemovedCoreEvent` is dispatched, because the UI is already gone. +5. `adapter.destroy()` (D5). +6. `ToolsManager.destroy()`: destroy each inline tool's shared instance once (D7), and stop any tool preparation still running (D4). It runs last because the adapter, the selection manager and the inline toolbar all use those instances until they're gone. + +`destroy()` never resolves a service from the container, because that could construct it. `initialize()` stores each service it resolves (`SelectionManager`, `BlockRenderer`, `UndoRedoManager`, the adapter, `ToolsManager`) in a `#services` record, and teardown only destroys what that record holds. Each step runs through a helper that logs a throwing step and continues. + +The model is never mutated. If `destroy()` cleared blocks through the model, `CollaborationManager` would broadcast the removals to other clients and `UndoRedoManager` would record them. + +### D4. Destroy during a pending `initialize()` +`initialize()` awaits `#initializeTools()`, which runs each tool's static `prepare()` one after another through a queue. `destroy()` can land while one of them is pending. + +`ToolsManager` owns an `AbortController` that its `destroy()` aborts. Each queued job checks the signal before calling `prepare()`, so tools still waiting are never prepared. It checks again once `prepare()` settles, so a tool whose preparation finishes after destroy is not created, validated or registered, and no `ToolLoaded` is dispatched for it. An inline instance created then would never be destroyed, because `ToolsManager.destroy()` has already run. + +A `prepare()` that is already running can't be interrupted, because it takes no signal, and passing one would change the tool API. Its result is discarded instead. `initialize()` therefore settles once that `prepare()` settles, not immediately. Racing the queue against the signal would end the wait sooner without stopping the tool's work, so it isn't worth the extra code. + +After the queue settles, `initialize()` checks `#destroyed` and stops before the document is initialized or `core:ready` is dispatched. `destroy()` tears down whatever stage was reached. + +`initialize()` then **rejects** with a `DOMException` named `AbortError`, following the platform convention (`fetch` rejects the same way when its signal aborts). Code awaiting readiness (loading data, focusing, subscribing) must not run against a destroyed editor, and resolving would let it run. Callers that unmount during boot ignore the error by name: `if (error.name === 'AbortError') return`. This matters in practice: React StrictMode mounts, unmounts and remounts every component in development, so every React integration hits this path. + +`EditorJS` creates its ready promise in the constructor, so an `AbortError` would be reported as an unhandled rejection whenever nobody awaits `isReady`. The bundle attaches a handler to its own ready promise that ignores `AbortError` and logs any other error with `console.error`. Neither is reported as an unhandled rejection, a real initialization failure isn't lost when nobody awaits `isReady`, and anyone awaiting `isReady` sees the rejection either way. + +*Alternative*: rethrow non-abort errors from the handler so Node reports them as unhandled. Rejected because the rethrown copy is a separate promise: a caller who awaits and handles `isReady` would still get an unhandled rejection reported, and Jest or Node with `--unhandled-rejections=throw` would crash on it. The cost of logging is one extra `console.error` for callers who also handle the error themselves. + +*Alternative*: resolve on interruption. Rejected because `isReady` resolving would no longer mean the editor is usable, and the caller couldn't tell "ready" from "cancelled". + +### D5. Adapter plugin gets a required `destroy()` +`EditorJSAdapterPlugin` narrows `destroy?()` to `destroy()`. Core can't release the rendering layer without it, and `DOMAdapters` is the only implementation. `DOMAdapters.destroy()`: +- destroys each remaining entry in `#adapters` through the existing `destroyBlockToolAdapter` path +- calls `CaretAdapter.destroy()`, which aborts its controller. That controller's `abort` event is tied to `off(holder)` on the selection watcher and to the `api.selection.onCaretUpdate` unsubscribe function +- calls `FormattingAdapter.destroy()`, which aborts its controller (covering the `core:ToolLoaded` and `onUpdate` listeners) and drops its references to the inline tools. It doesn't destroy them: they're shared (D7) + +Singletons are fetched from its private container, where they already exist, so nothing new gets constructed. + +### D6. Reference-count the `selectionchange` watcher +`useSelectionChange` is a module-level singleton shared by every editor on the page, so its `destroy()` would break the other editors. Instead, `off()` removes the `document` listener when the subscriber map becomes empty, and `on()` adds it back when it adds the first subscriber. This lets "no `document` listener left after every editor is destroyed" pass, and it is safe when several editors run at once. + +### D7. Tool instance lifetime rule +Add `destroy?(): void` to `InlineTool`. `BlockTool` already gets it from the v2 type, and we spell it out in the SDK spec. The rule is that every instance the editor creates gets `destroy()` called once, by whoever owns it, when the editor stops using it. + +The owner differs by tool type, because the facades create instances differently: +- **Block tools**: `BlockToolFacade.create()` builds a new instance per block, and `BlockRenderer` owns it. It calls `destroy()` when the block is removed or on teardown. +- **Inline tools**: `InlineToolFacade.create()` returns one cached instance per tool (since #127), so `FormattingAdapter`, `SelectionManager.applyInlineTool`, `ToolsManager` validation and `InlineToolbarUI` all receive the same object. None of them owns it, and none of them destroys it. The facade does: `InlineToolFacade.destroy()` destroys the instance once (only if one was ever created) and drops it. `ToolsManager.destroy()` calls it for every inline tool as the last step of teardown (D3), logging a throwing hook and moving on to the next tool. + +So an inline tool's `destroy()` runs exactly once, when the editor is destroyed. It doesn't run when the inline toolbar is rebuilt or hidden, or after a format is applied. + +*Alternative*: each consumer destroys the instance it was handed. Rejected because the instance is shared. The first consumer to finish would destroy it under the others, and teardown would call the hook once per consumer. That's how this change was first written, and review caught it. + +*Alternative*: make `InlineToolFacade.create()` return a new instance each time, like block tools. Rejected because tools may rely on keeping state between toolbar renders and applications, and that would be a behaviour change outside this work. + +### D8. Listeners are released through `AbortController` across the repo +Every first-party component that registers listeners owns one `AbortController`. It passes the controller's `signal` to every registration and calls `abort()` in its teardown. This covers: +- `EventTarget` registrations: the EventBus, the model, DOM nodes, `document`. +- Subscriptions that return an unsubscribe function: `api.document.onUpdate`, `api.selection.onCaretUpdate`, and the `selectionchange` watcher (which gets `off(holder)` wrapped in a closure). The component registers that function as an `abort` listener on its own signal (a signal aborts only once, so no `{ once: true }` is needed), as in `signal.addEventListener('abort', api.document.onUpdate(cb))`, so nothing is stored in a field. + +No component keeps handler fields or unsubscribe functions just so it can remove them later. `UndoRedoManager`, `ClipboardPlugin`, `ShortcutsPlugin`, `CollaborationManager`, and `DOMBlockToolAdapter` move to this pattern. Non-listener resources (timers, the socket, DOM nodes, popovers, tool instances) are still released explicitly in `destroy()`. + +Needed plumbing: +- `EditorJSModel`'s typed `addEventListener` overloads take an `options?: AddEventListenerOptions` argument and forward it. +- Core's `DocumentAPI.onUpdate` and `SelectionAPI.onCaretUpdate` create a controller per subscription, pass its signal to `model.addEventListener`, and return `() => controller.abort()`. Their public signatures don't change. + +*Alternative*: add an optional `{ signal }` argument to `onUpdate`/`onCaretUpdate`. Rejected because it makes the public API bigger for something one line at the call site already handles. + +*Alternative*: store handlers and remove them by hand, the way `UndoRedoManager` does today. Rejected because a forgotten removal leaks without any warning, and the UI alone would need about 15 handler fields. + +### D8a. SDK base classes own the controller +When the SDK provides a base class that registers listeners, the teardown belongs to the base class, not to its subclasses. Today `BlockToolAdapter` is the only such class. It creates the controller, ties the `onUpdate` unsubscribe function to the controller's `abort` event, exposes `protected readonly signal: AbortSignal`, and aborts it in `destroy()`. `DOMBlockToolAdapter` passes `this.signal` to its `ui:beforeinput` registration and drops its `destroy()` override and its `#beforeInputListener` field. A subclass overrides `destroy()` only to release resources that aren't listeners, and it still calls `super.destroy()`. + +Plugins and tools are interfaces, not base classes, so they own their own controllers. *Alternative*: Core passes each plugin a `signal` in `EditorjsPluginParams` and aborts it after the plugin's `destroy()`. Rejected because it adds to the plugin API. + +### D9. The `EditorJS` bundle delegates +`EditorJS#destroy()` calls `this.#core.destroy()`. It owns nothing else. + +## Risks / Trade-offs + +- [A plugin's `destroy()` throws partway through] → Each plugin call is wrapped in its own try/catch and logged, and teardown continues. The editor may still be left partly torn down, which beats aborting and leaking everything. +- [An async `render()` settles after teardown and dispatches `BlockAddedCoreEvent` into a dead EventBus] → `BlockRenderer` checks a `#destroyed` flag after the `await`, then destroys the late instance and its adapter instead of dispatching. +- [Ops not yet sent in `CollaborationManager`'s debounced batch are lost on destroy] → This already happens when a tab closes. Flushing would need async teardown (Non-Goal) and is listed in Open Questions. +- [The adapter contract narrows `destroy?()` to `destroy()`, which is a type-level break for third-party adapters] → No third-party adapters exist yet, and the package is pre-1.0. +- [#174 overlaps with UI teardown] → #174 covers every UI plugin's teardown, so this change has no UI plugin implementation. `InlineToolbarUI` doesn't destroy inline tool instances either, because they're shared (D7). Both changes edit the same `docs/plugins.md` lifecycle line, so whichever lands second merges that one line. + +## Migration Plan + +Additive for integrators: call `editor.destroy()` on unmount. No data migration and nothing to roll back beyond reverting the PR. + +## Open Questions + +- Should `destroy()` become async later, to flush the pending collaboration batch and await socket close? If so, `EditorjsPlugin.destroy` would need to return `void | Promise`. +- Should the API expose a `core:destroyed` event for third-party code that subscribed through `api` without being a plugin? It isn't needed for the built-ins. diff --git a/openspec/changes/editor-destroy/proposal.md b/openspec/changes/editor-destroy/proposal.md new file mode 100644 index 00000000..38bb4f60 --- /dev/null +++ b/openspec/changes/editor-destroy/proposal.md @@ -0,0 +1,37 @@ +## Why + +There is no way to tear an editor down. Neither `Core` nor `EditorJS` has a `destroy()`, so an integrator who unmounts an editor (SPA route change, React/Vue component unmount, several editors on one page) leaks the editor DOM, a document-wide `selectionchange` listener, the collaboration WebSocket, timers, and every tool and plugin instance. Several plugins already implement `destroy()` and the `core` and `sdk` specs already describe it being called on teardown ("Registry is cleared on teardown", "Destroying a plugin"), but nothing calls it. This change makes those requirements true. + +## What Changes + +- Add `destroy(): void` to `Core`. It is synchronous, safe to call more than once, and safe to call while `initialize()` is still pending, in which case the pending `initialize()` rejects with an `AbortError`. After it returns, `use()` and `initialize()` throw. +- `Core` keeps the plugin instances it creates. `destroy()` calls `destroy()` on each of them in reverse construction order and removes each one's entry from the plugin registry. A plugin whose `destroy()` throws is logged and doesn't stop the rest of the teardown. +- `Core` tears down its own services: `UndoRedoManager` (already has `destroy()`), `BlockRenderer` and `SelectionManager` (new), each removing its model and EventBus listeners. +- `BlockRenderer` keeps the block tool instance for each rendered block. When a block is removed, and when the editor is destroyed, it calls the tool's `destroy()` and destroys the block's adapter. Teardown does not touch the model, so no removal operations reach undo history or collaboration. +- Add an optional `destroy()` to the `InlineTool` contract in `@editorjs/sdk`. `BlockTool` already inherits an optional `destroy()` from the v2 type; the SDK spec now names both. The editor calls `destroy()` once on each tool instance it creates, when it stops using that instance. Inline tools have one instance shared by all consumers, so their facade destroys it, through `ToolsManager`, as the last step of teardown. +- Add `destroy()` to the adapter plugin. `DOMAdapters.destroy()` destroys the remaining block adapters, stops `CaretAdapter` watching selection changes and caret updates, and stops `FormattingAdapter` listening to tool and model events. The shared `selectionchange` watcher removes its `document` listener once its last subscriber is gone. +- UI plugin teardown (every UI plugin unsubscribing, removing its DOM and popovers, and `EditorjsUI` removing its wrapper while leaving the holder) is covered by #174, so this change has no UI plugin implementation. +- Listeners are released through `AbortController` across the repo. Each component owns one controller, passes its `signal` to every listener it registers, and calls `abort()` on teardown. When an SDK base class registers listeners (`BlockToolAdapter`), the base class owns the controller, gives subclasses a protected `signal`, and aborts it in its own `destroy()`. Subclasses don't override `destroy()` to remove listeners. +- The `EditorAPI` subscription methods keep their signatures. Internally `onUpdate` and `onCaretUpdate` subscribe through their own controller and return `() => controller.abort()`. Components tie that function to their own controller's `abort` event. +- Add `destroy()` to the `EditorJS` bundle, which delegates to `Core.destroy()`. Because `CollaborationManager.destroy()` already closes the `OTClient` socket, the bundle closes its collaboration connection too. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `architecture`: adds the repo-wide rule that listeners are released through `AbortController`, owned by the SDK base class where one exists. +- `core`: adds an editor teardown requirement to `Core` (idempotency, ordering, mid-initialization destroy, post-destroy guards) and adds per-block tool destruction to the block rendering lifecycle. +- `sdk`: `BlockToolAdapter` owns its controller and exposes `signal` to subclasses, the tool contracts gain an optional `destroy()` on block and inline tools, and the adapter plugin contract gains a teardown that covers adapters not destroyed yet. +- `dom-adapters`: `DOMAdapters` releases the caret, selection, and formatting subscriptions and the block adapters on destroy, and the shared selection watcher detaches from `document` when it has no subscribers left. +- `editorjs-bundle`: `EditorJS` exposes `destroy()`. + +## Impact + +- **Code**: `packages/core/src/index.ts`, `packages/core/src/components/{BlockRenderer,SelectionManager}.ts`, `packages/core/src/tools/ToolsManager.ts`, `packages/sdk/src/entities/{InlineTool,EditorjsAdapterPlugin,BlockToolAdapter}.ts`, `packages/sdk/src/tools/facades/InlineToolFacade.ts`, `packages/core/src/api/{DocumentAPI/DocumentAPI,SelectionAPI}.ts`, `packages/core/src/components/UndoRedoManager.ts`, `packages/model/src/EditorJSModel.ts` (`addEventListener` overloads gain `options`), `packages/plugins/{clipboard-plugin,shortcuts-plugin}`, `packages/collaboration-manager/src/CollaborationManager.ts`, `packages/dom-adapters/src/{index.ts,CaretAdapter,FormattingAdapter,utils/useSelectionChange.ts}`, `packages/editorjs/src/index.ts`. +- **Public API**: adds `Core#destroy()`, `EditorJS#destroy()`, optional `InlineTool#destroy()`, and `EditorJSAdapterPlugin#destroy()`. Nothing breaks: the tool hooks are optional, and `DOMAdapters` is the only adapter implementation in the repo. +- **Docs**: `docs/plugins.md` ("Lifecycle boundary") says "`Core` currently does not expose a global `destroy()` lifecycle hook". This change replaces that sentence. `docs/README.md` ("Lifecycle") gains a teardown step. +- **Related work**: #174 (UI plugin teardown), card "Clean up" on the DocumentModel board. diff --git a/openspec/changes/editor-destroy/specs/architecture/spec.md b/openspec/changes/editor-destroy/specs/architecture/spec.md new file mode 100644 index 00000000..e8633ab1 --- /dev/null +++ b/openspec/changes/editor-destroy/specs/architecture/spec.md @@ -0,0 +1,21 @@ +## ADDED Requirements + +### Requirement: Listener teardown through AbortController +Every first-party component that registers event listeners (core services, adapters, plugins, UI, and SDK base classes) SHALL own an `AbortController`, pass its `signal` to each listener registration it makes, and release all of those listeners by calling `abort()` in its teardown. This applies to `EventTarget` registrations (EventBus, the model, DOM nodes, `document`, `window`). For subscriptions that return an unsubscribe function (`api.document.onUpdate`, `api.selection.onCaretUpdate`, the `selectionchange` watcher), the component SHALL register that function as an `abort` listener on its controller's `signal`. A component SHALL NOT store handler references or unsubscribe functions in fields only to remove listeners. Resources that aren't listeners, such as timers, sockets, DOM nodes, popovers, and tool instances, are released explicitly. + +When an SDK base class registers listeners, the base class SHALL own the controller, expose its `signal` to subclasses as a protected member, and abort it in its own `destroy()`. Subclasses SHALL pass that `signal` to their own registrations rather than creating a second controller or overriding `destroy()` to remove listeners. + +#### Scenario: Teardown removes every listener at once +- **GIVEN** a component that registered listeners on the EventBus, the model, and a DOM node using its controller's `signal` +- **WHEN** its teardown calls `abort()` +- **THEN** none of those listeners fire for events dispatched afterwards + +#### Scenario: Subscription that returns an unsubscribe function +- **GIVEN** a component subscribed through `api.document.onUpdate` and registered the returned function as an `abort` listener on its controller's `signal` +- **WHEN** the component's teardown calls `abort()` +- **THEN** the subscription is removed, and later model changes don't reach its callback + +#### Scenario: Subclass of an SDK base class +- **GIVEN** a class extending an SDK base class registers an extra EventBus listener with the inherited `signal` +- **WHEN** the base class's `destroy()` runs +- **THEN** both the base class's listeners and the subclass's listener are removed, and the subclass doesn't need to override `destroy()` to do it diff --git a/openspec/changes/editor-destroy/specs/core/spec.md b/openspec/changes/editor-destroy/specs/core/spec.md new file mode 100644 index 00000000..cf09fa95 --- /dev/null +++ b/openspec/changes/editor-destroy/specs/core/spec.md @@ -0,0 +1,86 @@ +## ADDED Requirements + +### Requirement: Editor teardown +`Core` SHALL expose a synchronous `destroy()` that releases everything the editor created: plugin instances, the adapter plugin, block and inline tool instances, block adapters, and every model, EventBus, DOM, timer, and network subscription that editor-owned code registered. Teardown SHALL NOT mutate the document model, so no removal events reach undo history, collaboration, or `onModelUpdate`. The holder element passed in the configuration belongs to the integrator and SHALL be left in the DOM. + +#### Scenario: Plugins are destroyed in reverse construction order +- **GIVEN** plugins A, B, and C were constructed in that order during `initialize()` +- **WHEN** `destroy()` is called +- **THEN** `destroy()` is called on C, then B, then A, and plugins that don't implement `destroy()` are skipped + +#### Scenario: Plugin registry entries are removed +- **GIVEN** a plugin registered a `publicApi` under its `name` +- **WHEN** `destroy()` is called +- **THEN** that name is no longer present on `api.plugins` + +#### Scenario: A failing plugin does not abort teardown +- **GIVEN** one plugin's `destroy()` throws +- **WHEN** `destroy()` is called +- **THEN** the error is logged with `console.error`, and every other plugin, the core services, the rendered blocks, and the adapter plugin are still torn down + +#### Scenario: Core services stop listening +- **GIVEN** an initialized editor +- **WHEN** `destroy()` is called +- **THEN** `UndoRedoManager`, `BlockRenderer`, and `SelectionManager` remove their model and EventBus listeners, so model changes or `core:undo`/`core:redo` events dispatched afterwards have no effect on them + +#### Scenario: Rendered blocks are released without touching the model +- **GIVEN** an initialized editor with rendered blocks +- **WHEN** `destroy()` is called +- **THEN** each rendered block tool's `destroy()` is called, each block's adapter is destroyed, the adapter plugin's `destroy()` is called, and the model emits no events + +#### Scenario: Shared inline tool instances are destroyed last, once each +- **GIVEN** an initialized editor whose inline tools have been used by the formatting adapter, the selection manager and the inline toolbar +- **WHEN** `destroy()` is called +- **THEN** each inline tool instance's `destroy()` is called once, after the adapter plugin's `destroy()`; a throwing hook is logged with `console.error` and the other inline tools are still destroyed; inline tools that were never instantiated are not constructed + +#### Scenario: Destroying twice is a no-op +- **GIVEN** `destroy()` has already been called +- **WHEN** `destroy()` is called again +- **THEN** it returns without calling any plugin, tool, or adapter `destroy()` a second time + +#### Scenario: Destroying before initialization +- **GIVEN** `use()` has been called but `initialize()` has not +- **WHEN** `destroy()` is called +- **THEN** it completes without error and no plugin or tool is constructed + +#### Scenario: Destroying while initialization is pending +- **GIVEN** `initialize()` has been called and its promise has not settled +- **WHEN** `destroy()` is called +- **THEN** whatever was constructed so far is torn down, the pending `initialize()` rejects with a `DOMException` named `AbortError` without initializing the document or dispatching `CoreEventType.Ready`, any tool or plugin it would have constructed afterwards is not constructed, tools still queued for preparation are not prepared, and a tool whose `prepare()` settles after `destroy()` is neither created nor registered and dispatches no `CoreEventType.ToolLoaded` + +#### Scenario: Using a destroyed editor +- **GIVEN** `destroy()` has been called +- **WHEN** `use()` or `initialize()` is called +- **THEN** it throws an error stating that the editor has been destroyed + +## MODIFIED Requirements + +### Requirement: Block rendering lifecycle +The system SHALL provide `BlockRenderer`, which listens for model `BlockAddedEvent`/`BlockRemovedEvent`, creates a `BlockToolAdapter` per block, instantiates and renders the corresponding tool, and dispatches `BlockAddedCoreEvent`/`BlockRemovedCoreEvent`. `BlockRenderer` SHALL keep the tool instance of each rendered block, keyed by block id, and SHALL call that instance's optional `destroy()` exactly once, either when the block is removed or when the editor is destroyed. + +#### Scenario: Malformed or unknown block events +- **GIVEN** a block event with an undefined index, or referencing a tool name that isn't registered +- **WHEN** `BlockRenderer` processes the event +- **THEN** it throws `[BlockRenderer] Block index should be defined...` or `[BlockRenderer] Block Tool not found` respectively + +#### Scenario: Tool render failure is logged, not thrown +- **GIVEN** a tool's `render()` method returns a rejected promise +- **WHEN** `BlockRenderer` renders that block +- **THEN** the rejection is logged rather than propagated as an uncaught error + +#### Scenario: Removing a block destroys its tool instance +- **GIVEN** a rendered block whose tool instance implements `destroy()` +- **WHEN** the model emits `BlockRemovedEvent` for that block +- **THEN** the tool instance's `destroy()` is called, the block's adapter is destroyed, and `BlockRemovedCoreEvent` is dispatched + +#### Scenario: Destroying the renderer releases every remaining block +- **GIVEN** several rendered blocks +- **WHEN** `BlockRenderer.destroy()` is called +- **THEN** each remaining tool instance's `destroy()` is called, each remaining block's adapter is destroyed, no `BlockRemovedCoreEvent` is dispatched, and the renderer stops listening to model events + +#### Scenario: A block that finishes rendering after teardown +- **GIVEN** a block tool's `render()` is still pending when `BlockRenderer.destroy()` is called +- **WHEN** `render()` settles +- **THEN** no `BlockAddedCoreEvent` is dispatched for it, and the tool instance and its adapter are still destroyed + +Implemented in `src/components/BlockRenderer.ts`, validated by its co-located `.spec.ts`. diff --git a/openspec/changes/editor-destroy/specs/dom-adapters/spec.md b/openspec/changes/editor-destroy/specs/dom-adapters/spec.md new file mode 100644 index 00000000..e641e2b2 --- /dev/null +++ b/openspec/changes/editor-destroy/specs/dom-adapters/spec.md @@ -0,0 +1,24 @@ +## ADDED Requirements + +### Requirement: Adapter teardown +`DOMAdapters` SHALL implement `destroy()`. It SHALL destroy every `DOMBlockToolAdapter` still alive and clear the `InputsRegistry`, stop `CaretAdapter` watching DOM selection changes and model caret updates, and stop `FormattingAdapter` listening to `core:ToolLoaded` and model updates. `FormattingAdapter` SHALL drop its inline tools without destroying them, because their instances are shared and their facades destroy them. The shared `selectionchange` watcher SHALL remove its `document` listener once its last subscriber unsubscribes, and add it back on the next subscription. + +#### Scenario: Caret adapter stops tracking the holder +- **GIVEN** an initialized `DOMAdapters` +- **WHEN** `destroy()` is called and a `selectionchange` event then fires inside the holder +- **THEN** the model caret isn't updated, and later model caret updates don't change the DOM selection + +#### Scenario: Remaining block adapters are destroyed +- **GIVEN** block adapters created for several blocks that haven't been removed +- **WHEN** `destroy()` is called +- **THEN** each adapter's `destroy()` runs, removing its model and `ui:beforeinput` listeners, and the inputs registry is empty + +#### Scenario: Formatting adapter releases inline tools +- **GIVEN** inline tools attached to `FormattingAdapter` through `core:ToolLoaded` +- **WHEN** `destroy()` is called +- **THEN** no inline tool instance's `destroy()` is called, the adapter holds none of them anymore, and later model formatting events don't touch the DOM + +#### Scenario: Two editors on one page +- **GIVEN** two editors are initialized on the same page and share the `selectionchange` watcher +- **WHEN** the first editor is destroyed +- **THEN** the second editor still receives selection changes, and the `document` listener is removed only after the second editor is destroyed too diff --git a/openspec/changes/editor-destroy/specs/editorjs-bundle/spec.md b/openspec/changes/editor-destroy/specs/editorjs-bundle/spec.md new file mode 100644 index 00000000..886d6a5e --- /dev/null +++ b/openspec/changes/editor-destroy/specs/editorjs-bundle/spec.md @@ -0,0 +1,29 @@ +## ADDED Requirements + +### Requirement: Editor teardown +The `EditorJS` class SHALL expose a synchronous `destroy()` that delegates to the underlying `Core.destroy()`. This tears down the default UI, plugins, tools, DOM adapter, and the collaboration connection. + +#### Scenario: Destroying a ready editor +- **GIVEN** `editor.isReady` has resolved +- **WHEN** `editor.destroy()` is called +- **THEN** the editor's DOM is removed from the holder, its collaboration socket is closed, and no listener it added to `document` or the holder remains + +#### Scenario: Destroying before ready +- **GIVEN** `editor.isReady` has not settled +- **WHEN** `editor.destroy()` is called +- **THEN** `editor.isReady` rejects with a `DOMException` named `AbortError`, the editor never becomes interactive, and nothing is left in the holder + +#### Scenario: Destroying before ready without awaiting readiness +- **GIVEN** nobody awaits or handles `editor.isReady` +- **WHEN** `editor.destroy()` is called before it settles +- **THEN** the `AbortError` rejection is not reported as an unhandled promise rejection + +#### Scenario: Initialization failure without awaiting readiness +- **GIVEN** nobody awaits or handles `editor.isReady` +- **WHEN** initialization fails with an error other than `AbortError` +- **THEN** that error is logged with `console.error`, is not reported as an unhandled promise rejection, and `editor.isReady` still rejects with it for anyone who awaits it + +#### Scenario: Several editors on one page +- **GIVEN** several `EditorJS` instances are mounted on one page +- **WHEN** each of them is destroyed +- **THEN** each destroy leaves the others working, and once all are destroyed no editor DOM nodes or editor-registered `document`/holder listeners remain diff --git a/openspec/changes/editor-destroy/specs/sdk/spec.md b/openspec/changes/editor-destroy/specs/sdk/spec.md new file mode 100644 index 00000000..82a302b6 --- /dev/null +++ b/openspec/changes/editor-destroy/specs/sdk/spec.md @@ -0,0 +1,56 @@ +## ADDED Requirements + +### Requirement: Tool instance teardown hook +The `BlockTool` and `InlineTool` instance contracts SHALL each declare an optional `destroy(): void`. The editor SHALL call `destroy()` exactly once on every tool instance it creates, when it stops using that instance. A block tool instance belongs to one block and SHALL be destroyed when that block is removed or the editor is destroyed. An inline tool facade SHALL hand the same instance to every consumer. Consumers SHALL NOT destroy it. `InlineToolFacade.destroy()` SHALL destroy it once, and only if it was created. A tool that doesn't implement `destroy()` SHALL keep working unchanged. + +#### Scenario: Inline tool declares destroy +- **GIVEN** an inline tool class implements `destroy()` +- **WHEN** it is type-checked against `InlineToolConstructor` +- **THEN** it compiles, and an inline tool without `destroy()` also compiles + +#### Scenario: The inline tool facade destroys its shared instance once +- **GIVEN** an inline tool facade whose `create()` has been called several times +- **WHEN** `destroy()` is called on the facade, once or more +- **THEN** every `create()` returned the same instance, its `destroy()` is called once, and a facade whose `create()` was never called constructs nothing + +#### Scenario: Block tool declares destroy +- **GIVEN** a block tool class implements `destroy()` +- **WHEN** it is type-checked against `BlockToolConstructor` +- **THEN** it compiles, and a block tool without `destroy()` also compiles + +## MODIFIED Requirements + +### Requirement: BlockToolAdapter bridges model events to per-block DOM state +The system SHALL provide an abstract `BlockToolAdapter` (extending `EventTarget`) that subscribes to `api.document.onUpdate`, translates model-level `DataNodeAddedEvent`/`DataNodeRemovedEvent`/`ValueModifiedEvent` into adapter-level `KeyAddedEvent`/`KeyRemovedEvent`/`ValueNodeChangedEvent` dispatched on itself, and exposes `registerTextInputKey`, `registerValueKey`, `removeKey`, and `destroy()`. `BlockToolAdapter` SHALL own an `AbortController`, tie its `api.document.onUpdate` unsubscribe function to that controller's `abort` event, expose its `signal` to subclasses as a protected member, and abort it in `destroy()`. + +#### Scenario: Cleaning up an adapter +- **GIVEN** a `BlockToolAdapter` instance is subscribed to model updates +- **WHEN** `destroy()` is called +- **THEN** its controller aborts, so its model listener is removed and no further translated events are dispatched + +#### Scenario: Subclass listeners are removed by the base class +- **GIVEN** a `BlockToolAdapter` subclass registered a listener with the inherited `signal` +- **WHEN** `destroy()` is called on the subclass instance +- **THEN** that listener is removed too, even though the subclass doesn't override `destroy()` + +Implemented in `src/entities/BlockToolAdapter.ts`. + +### Requirement: Plugin contracts +The system SHALL define `EditorjsPlugin`/`EditorjsPluginConstructor` (generic UI-plugin contract with optional `destroy()`, an optional `publicApi` member, a static `type`, and a static `name`) and `EditorJSAdapterPlugin`/`EditorjsAdapterPluginConstructor` (singleton adapter plugin contract with `createBlockToolAdapter`/`destroyBlockToolAdapter` and a required `destroy()` that releases every block adapter not destroyed yet, along with the adapter's own subscriptions). + +#### Scenario: Destroying a plugin +- **GIVEN** a registered `EditorjsPlugin` implements an optional `destroy()` method +- **WHEN** the editor tears down +- **THEN** `destroy()` is called on the plugin instance to release its resources + +#### Scenario: Destroying the adapter plugin +- **GIVEN** an adapter plugin with block adapters created through `createBlockToolAdapter` +- **WHEN** the editor tears down +- **THEN** the adapter plugin's `destroy()` is called after every rendered block has been released, and it destroys any block adapter still alive + +#### Scenario: Plugin declares an id and a public API +- **GIVEN** a plugin class declares a static `name` and an instance `publicApi` member +- **WHEN** its type is checked against `EditorjsPluginConstructor` +- **THEN** the `name` literal and the `publicApi` type are both inferred, and a mismatch with the plugin's `EditorjsPluginApiMap` augmentation is a compile error + +Implemented in `src/entities/EditorjsPlugin.ts`, `src/entities/EditorjsAdapterPlugin.ts`, `src/entities/EntityType.ts`. diff --git a/openspec/changes/editor-destroy/tasks.md b/openspec/changes/editor-destroy/tasks.md new file mode 100644 index 00000000..545b1dce --- /dev/null +++ b/openspec/changes/editor-destroy/tasks.md @@ -0,0 +1,60 @@ +## 1. SDK contracts + +- [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) + +- [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 +- [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) + +- [ ] 3.1 Write failing tests in `useSelectionChange.spec.ts`: `should remove the document listener when the last subscriber unsubscribes`, and `should re-add the document listener when a subscriber is added after all were removed` +- [ ] 3.2 Make `off()` detach the `document` listener when the subscriber map empties, and `on()` attach it when the first subscriber is added + +## 4. DOMAdapters teardown + +- [ ] 4.1 Write failing `CaretAdapter` tests: `should stop updating the model caret on selectionchange after destroy`, and `should not touch the DOM selection on model caret updates after destroy` +- [ ] 4.2 Implement `CaretAdapter.destroy()` by aborting its controller, whose `abort` event is tied to `off(holder)` and the `onCaretUpdate` unsubscribe function. Remove the `@todo` +- [ ] 4.3 Write failing `FormattingAdapter` tests: `should not destroy the shared inline tool instances it uses`, `should ignore core:ToolLoaded after destroy`, and `should ignore model updates after destroy` +- [ ] 4.4 Implement `FormattingAdapter.destroy()`: abort its controller, then drop the inline tools without destroying them (design D7) +- [ ] 4.5 Write failing `DOMAdapters` tests: `should destroy every remaining block adapter on destroy`, `should leave the inputs registry empty after destroy`, and `should destroy caret and formatting adapters on destroy` +- [ ] 4.6 Implement `DOMAdapters.destroy()` + +## 5. Core services teardown + +- [x] 5.1 Write failing `BlockRenderer` tests: `should call tool destroy when its block is removed`, `should destroy every remaining tool and block adapter on destroy without dispatching BlockRemovedCoreEvent`, `should stop handling model events after destroy`, and `should not dispatch BlockAddedCoreEvent for a render that settles after destroy` +- [x] 5.2 Track tool instances per block id in `BlockRenderer`, subscribe to the model with a controller's signal, add `destroy()`, and remove the `@todo clear block tool adapter memory` +- [x] 5.3 Write a failing `SelectionManager` test, `should stop handling caret updates after destroy`. Implement `SelectionManager.destroy()` by aborting its controller +- [x] 5.4 Give `InlineToolFacade` a `destroy()` that destroys its shared instance once, and `ToolsManager` a `destroy()` that calls it for every inline tool, logging a throwing hook; `Core.destroy()` calls it last (design D3, D7). Test that `SelectionManager.applyInlineTool` and `ToolsManager` validation don't destroy the shared instance + +## 6. Core.destroy() + +- [x] 6.1 Write failing `Core` tests (`packages/core/src/index.spec.ts`): `should call plugin destroy in reverse construction order`, `should unregister plugin public APIs`, `should continue teardown and log when a plugin destroy throws`, `should destroy UndoRedoManager, SelectionManager, BlockRenderer and then the adapter`, `should not emit model events during destroy`, and `should be a no-op when called twice` +- [x] 6.2 Keep plugin instances in `#initializePlugin`, track the initialization stage, and implement `destroy()` in the order from design D3 +- [x] 6.3 Write failing tests: `should not construct anything when destroyed before initialize`, `should reject initialize with an AbortError without dispatching ready when destroyed mid-initialization`, and `should throw from use and initialize after destroy` +- [x] 6.4 Implement the `#destroyed` checks after each `await` in `initialize()`, and the post-destroy guards in `use()` and `initialize()` +- [x] 6.5 Write failing `ToolsManager` tests, `should not prepare the tools still queued` and `should neither create nor announce a tool whose prepare() settles after destroy`. Give `ToolsManager` an `AbortController` that `destroy()` aborts, and check its signal before and after each `prepare()` (design D4) + +## 7. UI + +- [ ] 7.1 UI plugin teardown is covered by #174: no UI plugin implementation in this change +- [ ] 7.2 After #174 lands, test that `InlineToolbarUI` never destroys the shared inline tool instance, through a real `InlineToolFacade`, across rebuild, hide, a superseded render and destroy + +## 8. Bundle and integration + +- [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) + +## 9. Docs and wrap-up + +- [ ] 9.1 Replace the "`Core` currently does not expose a global `destroy()`" line in `docs/plugins.md` with the teardown contract (order, idempotency, tools' `destroy()`), and add a teardown step to the lifecycle in `docs/README.md`. Update `docs/diagrams/plugin-lifecycle-flow.mmd` +- [ ] 9.2 Run `yarn lint` and the test suites for every touched package +- [ ] 9.3 Run `openspec validate editor-destroy --type change` and make sure it passes diff --git a/packages/core/src/api/DocumentAPI/DocumentAPI.spec.ts b/packages/core/src/api/DocumentAPI/DocumentAPI.spec.ts index c589ae66..d7019984 100644 --- a/packages/core/src/api/DocumentAPI/DocumentAPI.spec.ts +++ b/packages/core/src/api/DocumentAPI/DocumentAPI.spec.ts @@ -84,4 +84,47 @@ describe('DocumentAPI', () => { expect(dispatchEvent).toHaveBeenCalledWith(expect.objectContaining({ name: 'redo' })); }); }); + + describe('.onUpdate()', () => { + /** + * EventTarget stands in for the model, so listeners are really added and removed + * @param target - stand-in for the model + */ + const createAPI = (target: EventTarget): InstanceType => new DocumentAPI( + target as unknown as InstanceType, + {} as unknown as CoreConfigValidated, + { dispatchEvent } as unknown as EventBus + ); + + it('should call the callback on model update', () => { + const target = new EventTarget(); + const callback = jest.fn(); + + createAPI(target).onUpdate(callback); + target.dispatchEvent(new Event('update')); + + expect(callback).toHaveBeenCalledTimes(1); + }); + + it('should stop calling the callback after the returned function is called', () => { + const target = new EventTarget(); + const callback = jest.fn(); + + const unsubscribe = createAPI(target).onUpdate(callback); + + unsubscribe(); + target.dispatchEvent(new Event('update')); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('should subscribe through an abort signal', () => { + const target = new EventTarget(); + const addEventListener = jest.spyOn(target, 'addEventListener'); + + createAPI(target).onUpdate(jest.fn()); + + expect(addEventListener).toHaveBeenCalledWith('update', expect.any(Function), { signal: expect.any(AbortSignal) }); + }); + }); }); diff --git a/packages/core/src/api/DocumentAPI/DocumentAPI.ts b/packages/core/src/api/DocumentAPI/DocumentAPI.ts index e1e26b98..bb39fc1c 100644 --- a/packages/core/src/api/DocumentAPI/DocumentAPI.ts +++ b/packages/core/src/api/DocumentAPI/DocumentAPI.ts @@ -64,11 +64,11 @@ export class DocumentAPI implements DocumentApiInterface { * @param callback - callback called on model update */ public onUpdate(callback: (event: ModelEvents) => void): () => void { - this.#model.addEventListener(EventType.Changed, callback); + const controller = new AbortController(); - return () => { - this.#model.removeEventListener(EventType.Changed, callback); - }; + this.#model.addEventListener(EventType.Changed, callback, { signal: controller.signal }); + + return () => controller.abort(); } /** diff --git a/packages/core/src/api/SelectionAPI.spec.ts b/packages/core/src/api/SelectionAPI.spec.ts index cf8c5d0c..cffc34f4 100644 --- a/packages/core/src/api/SelectionAPI.spec.ts +++ b/packages/core/src/api/SelectionAPI.spec.ts @@ -50,4 +50,47 @@ describe('SelectionAPI', () => { }); }); }); + + describe('.onCaretUpdate()', () => { + /** + * EventTarget stands in for the model, so listeners are really added and removed + * @param target - stand-in for the model + */ + const createAPI = (target: EventTarget): InstanceType => new SelectionAPI( + selectionManager as unknown as InstanceType, + target as unknown as InstanceType, + {} as unknown as CoreConfigValidated + ); + + it('should call the callback on caret update', () => { + const target = new EventTarget(); + const callback = jest.fn(); + + createAPI(target).onCaretUpdate(callback); + target.dispatchEvent(new Event('update')); + + expect(callback).toHaveBeenCalledTimes(1); + }); + + it('should stop calling the callback after the returned function is called', () => { + const target = new EventTarget(); + const callback = jest.fn(); + + const unsubscribe = createAPI(target).onCaretUpdate(callback); + + unsubscribe(); + target.dispatchEvent(new Event('update')); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('should subscribe through an abort signal', () => { + const target = new EventTarget(); + const addEventListener = jest.spyOn(target, 'addEventListener'); + + createAPI(target).onCaretUpdate(jest.fn()); + + expect(addEventListener).toHaveBeenCalledWith('update', expect.any(Function), { signal: expect.any(AbortSignal) }); + }); + }); }); diff --git a/packages/core/src/api/SelectionAPI.ts b/packages/core/src/api/SelectionAPI.ts index 5e2b323a..174a4759 100644 --- a/packages/core/src/api/SelectionAPI.ts +++ b/packages/core/src/api/SelectionAPI.ts @@ -66,11 +66,11 @@ export class SelectionAPI implements SelectionApiInterface { * @param callback - callback for CaretManager updates */ public onCaretUpdate(callback: (event: CaretManagerEvents) => void): () => void { - this.#model.addEventListener(EventType.CaretManagerUpdated, callback); + const controller = new AbortController(); - return () => { - this.#model.removeEventListener(EventType.CaretManagerUpdated, callback); - }; + this.#model.addEventListener(EventType.CaretManagerUpdated, callback, { signal: controller.signal }); + + return () => controller.abort(); } /** diff --git a/packages/core/src/components/BlockRenderer.spec.ts b/packages/core/src/components/BlockRenderer.spec.ts index 4647ec1f..62bcedfd 100644 --- a/packages/core/src/components/BlockRenderer.spec.ts +++ b/packages/core/src/components/BlockRenderer.spec.ts @@ -251,3 +251,195 @@ describe('BlockRenderer (unit, mocked deps)', () => { }); }); }); + +describe('BlockRenderer lifecycle', () => { + beforeEach(() => { + (console.error as jest.Mock).mockClear(); + }); + + interface Setup { + renderer: InstanceType; + emit: (event: unknown) => Promise; + signal: () => AbortSignal | undefined; + dispatchEvent: jest.Mock; + adapter: { createBlockToolAdapter: jest.Mock; + destroyBlockToolAdapter: jest.Mock; }; + } + + /** + * Creates a renderer whose model honours the abort signal, like a real EventTarget + * @param createTool - returns the tool instance for each added block + */ + function setup(createTool: () => { render: () => Promise; + destroy?: () => void; }): Setup { + let listener: ((event: unknown) => void | Promise) | undefined; + let listenerSignal: AbortSignal | undefined; + + const model = { + addEventListener: jest.fn((_type: string, callback: (event: unknown) => void, options?: AddEventListenerOptions) => { + listener = callback; + listenerSignal = options?.signal; + }), + }; + const dispatchEvent = jest.fn(); + const adapter = { + createBlockToolAdapter: jest.fn(() => ({})), + destroyBlockToolAdapter: jest.fn(), + }; + const toolsManager = { + blockTools: { + get: jest.fn(() => ({ + name: 'tool', + create: createTool, + })), + }, + }; + + const renderer = new BlockRenderer( + model as unknown as InstanceType, + { dispatchEvent } as unknown as InstanceType, + toolsManager as unknown as InstanceType, + adapter as unknown as EditorJSAdapterPlugin + ); + + return { + renderer, + emit: async (event: unknown) => { + if (listenerSignal?.aborted === true) { + return; + } + + await listener?.(event); + }, + signal: () => listenerSignal, + dispatchEvent, + adapter, + }; + } + + const added = (id: string, blockIndex = 0): unknown => new BlockAddedEvent( + { blockIndex } as unknown as BlockIndex, + { name: 'tool', + id: id as BlockId, + data: {} }, + USER_ID + ); + + const removed = (id: string, blockIndex = 0): unknown => new BlockRemovedEvent( + { blockIndex } as unknown as BlockIndex, + { name: 'tool', + id: id as BlockId, + data: {} }, + USER_ID + ); + + it('should call tool destroy when its block is removed', async () => { + const destroy = jest.fn(); + const { emit, adapter, dispatchEvent } = setup(() => ({ render: () => Promise.resolve({}), + destroy })); + + await emit(added('a')); + await emit(removed('a')); + + expect(destroy).toHaveBeenCalledTimes(1); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('a'); + expect(dispatchEvent).toHaveBeenCalledTimes(2); + }); + + it('should destroy every remaining tool and block adapter on destroy without dispatching BlockRemovedCoreEvent', async () => { + const destroy = jest.fn(); + const { renderer, emit, adapter, dispatchEvent } = setup(() => ({ render: () => Promise.resolve({}), + destroy })); + + await emit(added('a', 0)); + await emit(added('b', 1)); + dispatchEvent.mockClear(); + + renderer.destroy(); + + expect(destroy).toHaveBeenCalledTimes(2); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('a'); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('b'); + expect(dispatchEvent).not.toHaveBeenCalled(); + }); + + it('should not destroy a removed block again on destroy', async () => { + const destroy = jest.fn(); + const { renderer, emit } = setup(() => ({ render: () => Promise.resolve({}), + destroy })); + + await emit(added('a')); + await emit(removed('a')); + renderer.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('should stop handling model events after destroy', async () => { + const { renderer, emit, signal, dispatchEvent } = setup(() => ({ render: () => Promise.resolve({}) })); + + renderer.destroy(); + await emit(added('a')); + + expect(signal()?.aborted).toBe(true); + expect(dispatchEvent).not.toHaveBeenCalled(); + }); + + it('should not dispatch BlockAddedCoreEvent for a render that settles after destroy', async () => { + let resolveRender: (value: unknown) => void = () => undefined; + const destroy = jest.fn(); + const { renderer, emit, adapter, dispatchEvent } = setup(() => ({ + render: () => new Promise((resolve) => { + resolveRender = resolve; + }), + destroy, + })); + + const pending = emit(added('a')); + + renderer.destroy(); + resolveRender({}); + await pending; + + expect(dispatchEvent).not.toHaveBeenCalled(); + expect(destroy).toHaveBeenCalledTimes(1); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('a'); + }); + + it('should keep tearing down when a tool destroy throws', async () => { + let calls = 0; + const { renderer, emit, adapter } = setup(() => ({ + render: () => Promise.resolve({}), + destroy: () => { + calls++; + throw new Error('broken tool'); + }, + })); + + await emit(added('a', 0)); + await emit(added('b', 1)); + + expect(() => renderer.destroy()).not.toThrow(); + expect(calls).toBe(2); + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledTimes(2); + expect(console.error).toHaveBeenCalledWith('[BlockRenderer] Block Tool failed to destroy block a', expect.any(Error)); + }); + + it('should not log when a tool has no destroy', async () => { + const { renderer, emit } = setup(() => ({ render: () => Promise.resolve({}) })); + + await emit(added('a')); + renderer.destroy(); + + expect(console.error).not.toHaveBeenCalled(); + }); + + it('should destroy the adapter of a removed block that has no tracked tool instance', async () => { + const { emit, adapter } = setup(() => ({ render: () => Promise.resolve({}) })); + + await emit(removed('unknown')); + + expect(adapter.destroyBlockToolAdapter).toHaveBeenCalledWith('unknown'); + expect(console.error).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/components/BlockRenderer.ts b/packages/core/src/components/BlockRenderer.ts index 79a93384..9b4d4e71 100644 --- a/packages/core/src/components/BlockRenderer.ts +++ b/packages/core/src/components/BlockRenderer.ts @@ -6,6 +6,7 @@ import { EventType, ModelEvents } from '@editorjs/sdk'; +import type { BlockId, BlockTool } from '@editorjs/sdk'; import 'reflect-metadata'; import { inject, injectable } from 'inversify'; import { TOKENS } from '../tokens.js'; @@ -53,6 +54,16 @@ export class BlockRenderer { */ #adapter: EditorJSAdapterPlugin; + /** + * Tool instance of each rendered block, destroyed when the block is removed or the renderer is destroyed + */ + #blocks = new Map(); + + /** + * Releases the model listener on destroy + */ + readonly #controller = new AbortController(); + /** * BlockRenderer constructor. * All parameters are injected through the IoC container. @@ -73,7 +84,19 @@ export class BlockRenderer { this.#adapter = adapter; // eslint-disable-next-line @typescript-eslint/no-misused-promises -- Need to bubble the promise up in case of errors - this.#model.addEventListener(EventType.Changed, event => this.#handleModelUpdate(event)); + this.#model.addEventListener(EventType.Changed, event => this.#handleModelUpdate(event), { signal: this.#controller.signal }); + } + + /** + * Stops listening to the model and releases every rendered block: its tool instance and its adapter. + * The model is left untouched and no BlockRemovedCoreEvent is dispatched + */ + public destroy(): void { + this.#controller.abort(); + + for (const id of this.#blocks.keys()) { + this.#releaseBlock(id); + } } /** @@ -112,7 +135,8 @@ export class BlockRenderer { throw new Error(`[BlockRenderer] Block Tool ${data.name} not found`); } - const blockToolAdapter = this.#adapter.createBlockToolAdapter(createBlockId(data.id), tool.name); + const blockId = createBlockId(data.id); + const blockToolAdapter = this.#adapter.createBlockToolAdapter(blockId, tool.name); const block = tool.create({ adapter: blockToolAdapter, @@ -121,9 +145,18 @@ export class BlockRenderer { readOnly: false, }); + this.#blocks.set(blockId, block); + try { const blockElement = await block.render(); + /** + * The renderer was destroyed while the block was rendering, so the block is already released + */ + if (this.#controller.signal.aborted) { + return; + } + this.#eventBus.dispatchEvent(new BlockAddedCoreEvent({ tool: tool.name, data: data.data, @@ -149,15 +182,30 @@ export class BlockRenderer { throw new Error('[BlockRenderer] Block index should be defined. Probably something wrong with the Editor Model. Please, report this issue'); } - this.#adapter.destroyBlockToolAdapter(createBlockId(data.id)); + this.#releaseBlock(createBlockId(data.id)); this.#eventBus.dispatchEvent(new BlockRemovedCoreEvent({ tool: data.name, index: blockIndex, })); + } + + /** + * Destroys the block's tool instance and its adapter. + * A throwing tool is logged so the rest of the teardown still runs + * @param id - id of the block to release + */ + #releaseBlock(id: BlockId): void { + const block = this.#blocks.get(id); + + this.#blocks.delete(id); + + try { + block?.destroy?.(); + } catch (error) { + console.error(`[BlockRenderer] Block Tool failed to destroy block ${id}`, error); + } - /** - * @todo clear block tool adapter memory - */ + this.#adapter.destroyBlockToolAdapter(id); } } diff --git a/packages/core/src/components/SelectionManager.spec.ts b/packages/core/src/components/SelectionManager.spec.ts index 8a475340..72e94668 100644 --- a/packages/core/src/components/SelectionManager.spec.ts +++ b/packages/core/src/components/SelectionManager.spec.ts @@ -542,4 +542,72 @@ describe('SelectionManager', () => { expect(selectionManager.selectedBlocks()).toEqual([{ id: 'b0' }]); }); }); + + describe('tool and listener teardown', () => { + /** + * Sets up a caret with one valid text segment + * @param segment - segment returned by the caret index + */ + const mockCaret = (segment: Record): void => { + jest.spyOn(model, 'getCaret') + .mockReturnValue({ + index: { getTextSegments: jest.fn(() => [segment]) }, + update: jest.fn(), + } as unknown as ReturnType); + jest.spyOn(model, 'getFragments').mockReturnValue([]); + jest.spyOn(model, 'format').mockImplementation(() => undefined); + }; + + it('should not destroy the shared inline tool instance after applying it', () => { + const destroy = jest.fn(); + const toolMock = { + getFormattingOptions: jest.fn(() => ({ action: 'format', + range: [0, 3] })), + destroy, + }; + + (toolsManager as unknown as { inlineTools: Map }).inlineTools = new Map([['bold', { create: () => toolMock }]]); + mockCaret({ blockIndex: 0, + dataKey: 'text', + textRange: [0, 3] }); + + selectionManager.applyInlineTool({ toolName: 'bold' as InlineToolName }); + + expect(destroy).not.toHaveBeenCalled(); + }); + + it('should not destroy the shared inline tool instance when applying it throws', () => { + const destroy = jest.fn(); + const toolMock = { getFormattingOptions: jest.fn(), + destroy }; + + (toolsManager as unknown as { inlineTools: Map }).inlineTools = new Map([['bold', { create: () => toolMock }]]); + mockCaret({ blockIndex: 0, + dataKey: 'text' }); + + expect(() => selectionManager.applyInlineTool({ toolName: 'bold' as InlineToolName })).toThrow(); + expect(destroy).not.toHaveBeenCalled(); + }); + + it('should stop handling caret updates after destroy', () => { + // @ts-expect-error - Mocked instance + const ownModel = new EditorJSModel(); + let signal: AbortSignal | undefined; + + ownModel.addEventListener = (_type: EventType, _callback: (e: Event) => void, options?: AddEventListenerOptions) => { + signal = options?.signal; + }; + + const manager = new SelectionManager( + { userId: 'user' } as unknown as CoreConfigValidated, + ownModel, + eventBus, + toolsManager + ); + + manager.destroy(); + + expect(signal?.aborted).toBe(true); + }); + }); }); diff --git a/packages/core/src/components/SelectionManager.ts b/packages/core/src/components/SelectionManager.ts index b128ebf2..eb3dda40 100644 --- a/packages/core/src/components/SelectionManager.ts +++ b/packages/core/src/components/SelectionManager.ts @@ -47,6 +47,11 @@ export class SelectionManager { */ #toolsManager: ToolsManager; + /** + * Releases the caret listener on destroy + */ + readonly #controller = new AbortController(); + /** * @param config - Editor's config * @param model - editor model instance @@ -64,7 +69,18 @@ export class SelectionManager { this.#eventBus = eventBus; this.#toolsManager = toolsManager; - this.#model.addEventListener(EventType.CaretManagerUpdated, (event: CaretManagerEvents) => this.#handleCaretManagerUpdate(event)); + this.#model.addEventListener( + EventType.CaretManagerUpdated, + (event: CaretManagerEvents) => this.#handleCaretManagerUpdate(event), + { signal: this.#controller.signal } + ); + } + + /** + * Stops listening to caret updates + */ + public destroy(): void { + this.#controller.abort(); } /** diff --git a/packages/core/src/components/UndoRedoManager.spec.ts b/packages/core/src/components/UndoRedoManager.spec.ts index a1893e4a..bfd10a79 100644 --- a/packages/core/src/components/UndoRedoManager.spec.ts +++ b/packages/core/src/components/UndoRedoManager.spec.ts @@ -163,42 +163,44 @@ describe('UndoRedoManager', () => { it('should register a listener for model Changed events', () => { expect(model.addEventListener).toHaveBeenCalledWith( EventType.Changed, - expect.any(Function) + expect.any(Function), + { signal: expect.any(AbortSignal) } ); }); it('should register an undo listener on the eventBus', () => { expect(eventBus.addEventListener).toHaveBeenCalledWith( 'core:undo', - expect.any(Function) + expect.any(Function), + { signal: expect.any(AbortSignal) } ); }); it('should register a redo listener on the eventBus', () => { expect(eventBus.addEventListener).toHaveBeenCalledWith( 'core:redo', - expect.any(Function) + expect.any(Function), + { signal: expect.any(AbortSignal) } ); }); }); describe('.destroy()', () => { - it('should remove the undo listener from eventBus', () => { - manager.destroy(); - - expect(eventBus.removeEventListener).toHaveBeenCalledWith( - 'core:undo', - expect.any(Function) - ); - }); + /** + * Collects the signals passed to every listener registration + * @param mock - mocked addEventListener + */ + const signalsOf = (mock: jest.Mock): (AbortSignal | undefined)[] => mock.mock.calls.map( + call => (call[2] as AddEventListenerOptions | undefined)?.signal + ); - it('should remove the redo listener from eventBus', () => { + it('should abort the signal of the undo and redo listeners', () => { manager.destroy(); - expect(eventBus.removeEventListener).toHaveBeenCalledWith( - 'core:redo', - expect.any(Function) - ); + const signals = signalsOf(eventBus.addEventListener); + + expect(signals).toHaveLength(2); + expect(signals.every(signal => signal?.aborted === true)).toBe(true); }); it('should clear the debounce timer', () => { @@ -211,13 +213,12 @@ describe('UndoRedoManager', () => { expect(clearTimeoutSpy).toHaveBeenCalled(); }); - it('should remove the model updates listener', () => { + it('should abort the signal of the model updates listener', () => { manager.destroy(); - expect(model.removeEventListener).toHaveBeenCalledWith( - 'model:changed', - expect.any(Function) - ); + const [signal] = signalsOf(model.addEventListener); + + expect(signal?.aborted).toBe(true); }); }); diff --git a/packages/core/src/components/UndoRedoManager.ts b/packages/core/src/components/UndoRedoManager.ts index 3bdf018d..8abb1e1d 100644 --- a/packages/core/src/components/UndoRedoManager.ts +++ b/packages/core/src/components/UndoRedoManager.ts @@ -41,11 +41,6 @@ export class UndoRedoManager { */ #model: EditorJSModel; - /** - * Editor's EventBus instance - */ - #eventBus: EventBus; - /** * Editor configuration (provides the current user id). */ @@ -79,36 +74,9 @@ export class UndoRedoManager { #isApplying = false; /** - * Undo Core Event listener. Stored to be removed on destroy - * @param event - undo core event + * Releases the manager's model and EventBus listeners on destroy */ - #undoListener = (event: UndoCoreEvent): void => { - if (event.defaultPrevented) { - return; - } - - this.undo(); - }; - - /** - * Redo Core Event listener. Stored to be removed on destroy - * @param event - redo core event - */ - #redoListener = (event: RedoCoreEvent): void => { - if (event.defaultPrevented) { - return; - } - - this.redo(); - }; - - /** - * Model updates listener. Stored to be removed on destroy - * @param e - model event - */ - #modelUpdatesListener = (e: ModelEvents): void => { - this.#handleEvent(e); - }; + readonly #controller = new AbortController(); /** * UndoRedoManager constructor. @@ -124,12 +92,26 @@ export class UndoRedoManager { ) { this.#config = config; this.#model = model; - this.#eventBus = eventBus; - model.addEventListener(EventType.Changed, this.#modelUpdatesListener); + const { signal } = this.#controller; + + model.addEventListener(EventType.Changed, (e: ModelEvents) => this.#handleEvent(e), { signal }); + + eventBus.addEventListener(`core:${CoreEventType.Undo}`, (event: UndoCoreEvent) => { + if (event.defaultPrevented) { + return; + } + + this.undo(); + }, { signal }); + + eventBus.addEventListener(`core:${CoreEventType.Redo}`, (event: RedoCoreEvent) => { + if (event.defaultPrevented) { + return; + } - eventBus.addEventListener(`core:${CoreEventType.Undo}`, this.#undoListener); - eventBus.addEventListener(`core:${CoreEventType.Redo}`, this.#redoListener); + this.redo(); + }, { signal }); } /** @@ -180,13 +162,11 @@ export class UndoRedoManager { } /** - * Releases resources held by the manager (the pending debounce timer). + * Releases resources held by the manager: its listeners and the pending debounce timer */ public destroy(): void { clearTimeout(this.#debounceTimer); - this.#eventBus.removeEventListener(`core:${CoreEventType.Undo}`, this.#undoListener); - this.#eventBus.removeEventListener(`core:${CoreEventType.Redo}`, this.#redoListener); - this.#model.removeEventListener(EventType.Changed, this.#modelUpdatesListener); + this.#controller.abort(); } /** diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index 3338540a..4fd4c8f3 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -1,8 +1,12 @@ /* eslint-disable jsdoc/require-jsdoc -- inline test stubs */ -import { describe, it, expect } from '@jest/globals'; -import { PluginType, ToolType } from '@editorjs/sdk'; -import type { CoreConfig } from '@editorjs/sdk'; +import { describe, it, expect, jest, beforeEach } from '@jest/globals'; +import { CoreEventType, PluginType, ToolType } from '@editorjs/sdk'; +import type { CoreConfig, EditorjsPluginParams } from '@editorjs/sdk'; import Core from './index.js'; +import { UndoRedoManager } from './components/UndoRedoManager.js'; +import { SelectionManager } from './components/SelectionManager.js'; +import { BlockRenderer } from './components/BlockRenderer.js'; +import ToolsManager from './tools/ToolsManager.js'; /** * `Core` is headless: it registers nothing by default, so `initialize()` validates @@ -107,4 +111,382 @@ describe('Core', () => { expect(StubPlugin.instances).toBe(1); }); }); + + describe('destroy()', () => { + /** + * Records teardown calls across plugins, tools and the adapter, in order + */ + let log: string[]; + + beforeEach(() => { + log = []; + }); + + /** + * Adapter recording its teardown + */ + const createAdapter = (): unknown => class RecordingAdapter { + public static type = PluginType.Adapter as const; + public static name = 'recording-adapter'; + + public createBlockToolAdapter(): object { + return {}; + } + + /** + * The v2 data import assigns block ids, so the id isn't recorded + */ + public destroyBlockToolAdapter(): void { + log.push('adapter:block'); + } + + public destroy(): void { + log.push('adapter'); + } + }; + + /** + * Block tool recording its construction and teardown + */ + const createBlockTool = (): unknown => class RecordingBlockTool { + public static type = ToolType.Block as const; + public static name = 'paragraph'; + + public render(): Promise { + return Promise.resolve({}); + } + + public destroy(): void { + log.push('tool'); + } + }; + + /** + * Plugin recording its teardown under the given name + * @param name - static name the plugin registers its public API under + * @param onCreate - receives the plugin's constructor params + */ + const createPlugin = (name: string, onCreate?: (params: EditorjsPluginParams) => void): unknown => class RecordingPlugin { + public static type = PluginType.Plugin as const; + public static name = name; + + public publicApi = { name }; + + constructor(params: EditorjsPluginParams) { + onCreate?.(params); + } + + public destroy(): void { + log.push(`plugin:${name}`); + } + }; + + /** + * Creates a core with one block of data and the recording adapter and block tool registered + * @param overrides - config overrides + */ + const createCore = (overrides: Partial = {}): Core => { + const core = new Core(createConfig({ + data: { blocks: [{ id: 'block-1', + type: 'paragraph', + data: {} }] }, + ...overrides, + } as Partial)); + + core.use(createAdapter() as typeof StubAdapter); + core.use(createBlockTool() as typeof StubBlockTool); + + return core; + }; + + /** + * Lets the block render promise settle + */ + const flush = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + + it('should call plugin destroy in reverse construction order', async () => { + const core = createCore(); + + core.use(createPlugin('a') as typeof StubPlugin); + core.use(createPlugin('b') as typeof StubPlugin); + core.use(createPlugin('c') as typeof StubPlugin); + await core.initialize(); + + core.destroy(); + + expect(log.filter(entry => entry.startsWith('plugin:'))).toEqual(['plugin:c', 'plugin:b', 'plugin:a']); + }); + + it('should unregister plugin public APIs', async () => { + let params: EditorjsPluginParams | undefined; + const core = createCore(); + + core.use(createPlugin('probe', (p) => { + params = p; + }) as typeof StubPlugin); + await core.initialize(); + + expect((params?.api.plugins as Record).probe).toBeDefined(); + + core.destroy(); + + expect((params?.api.plugins as Record).probe).toBeUndefined(); + }); + + it('should continue teardown and log when a plugin destroy throws', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = createCore(); + + core.use(createPlugin('a') as typeof StubPlugin); + core.use(class BrokenPlugin { + public static type = PluginType.Plugin as const; + public static name = 'broken'; + + public destroy(): void { + throw new Error('broken plugin'); + } + } as unknown as typeof StubPlugin); + await core.initialize(); + await flush(); + + expect(() => core.destroy()).not.toThrow(); + expect(error).toHaveBeenCalledWith('[Core] Failed to destroy plugin "broken"', expect.any(Error)); + expect(log).toEqual(['plugin:a', 'tool', 'adapter:block', 'adapter']); + + error.mockRestore(); + }); + + it('should destroy plugins, then rendered blocks, then the adapter', async () => { + const core = createCore(); + + core.use(createPlugin('a') as typeof StubPlugin); + await core.initialize(); + await flush(); + + core.destroy(); + + expect(log).toEqual(['plugin:a', 'tool', 'adapter:block', 'adapter']); + }); + + it('should destroy each shared inline tool instance once, after the adapter', async () => { + const core = createCore(); + + core.use(class SharedInlineTool { + public static type = ToolType.Inline as const; + public static name = 'shared'; + + /** + * Gives the tool a validation pass, which creates its shared instance + */ + public static prepare(): Promise { + return Promise.resolve(); + } + + public destroy(): void { + log.push('inline'); + } + } as unknown as typeof StubBlockTool); + await core.initialize(); + await flush(); + + core.destroy(); + core.destroy(); + + expect(log).toEqual(['tool', 'adapter:block', 'adapter', 'inline']); + }); + + it('should not emit model events during destroy', async () => { + const onModelUpdate = jest.fn(); + const core = createCore({ onModelUpdate }); + + await core.initialize(); + await flush(); + onModelUpdate.mockClear(); + + core.destroy(); + + expect(onModelUpdate).not.toHaveBeenCalled(); + }); + + it('should stop calling onModelUpdate after destroy', async () => { + let model: { removeBlock: (userId: string, index: number) => void } | undefined; + const onModelUpdate = jest.fn((updated: unknown) => { + model = updated as typeof model; + }); + const core = createCore({ onModelUpdate }); + + await core.initialize(); + await flush(); + core.destroy(); + onModelUpdate.mockClear(); + + // An integrator that kept the model it was handed changes it after teardown + model?.removeBlock('user', 0); + + expect(model).toBeDefined(); + expect(onModelUpdate).not.toHaveBeenCalled(); + }); + + it('should be a no-op when called twice', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = createCore(); + + core.use(createPlugin('a') as typeof StubPlugin); + await core.initialize(); + await flush(); + + core.destroy(); + core.destroy(); + + expect(log).toEqual(['plugin:a', 'tool', 'adapter:block', 'adapter']); + expect(error).not.toHaveBeenCalled(); + + error.mockRestore(); + }); + + it.each([ + ['UndoRedoManager', UndoRedoManager], + ['SelectionManager', SelectionManager], + ['BlockRenderer', BlockRenderer], + ['ToolsManager', ToolsManager], + ])('should destroy %s', async (_label, Service) => { + const destroy = jest.spyOn(Service.prototype, 'destroy'); + const core = createCore(); + + await core.initialize(); + core.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); + + destroy.mockRestore(); + }); + + it.each([ + ['UndoRedoManager', UndoRedoManager], + ['SelectionManager', SelectionManager], + ['BlockRenderer', BlockRenderer], + ['ToolsManager', ToolsManager], + ])('should log and continue when %s fails to destroy', async (label, Service) => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const destroy = jest.spyOn(Service.prototype, 'destroy').mockImplementation(() => { + throw new Error('broken service'); + }); + const core = createCore(); + + await core.initialize(); + core.destroy(); + + expect(error).toHaveBeenCalledWith(`[Core] Failed to destroy ${label}`, expect.any(Error)); + expect(log).toContain('adapter'); + + destroy.mockRestore(); + error.mockRestore(); + }); + + it('should log and continue when the adapter fails to destroy', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = new Core(createConfig()); + + core.use(class BrokenAdapter { + public static type = PluginType.Adapter as const; + + public destroy(): void { + throw new Error('broken adapter'); + } + } as unknown as typeof StubAdapter); + core.use(StubBlockTool); + await core.initialize(); + + core.destroy(); + + expect(error).toHaveBeenCalledWith('[Core] Failed to destroy adapter', expect.any(Error)); + + error.mockRestore(); + }); + + it('should not log when the adapter has no destroy', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = new Core(createConfig()); + + core.use(StubAdapter); + core.use(StubBlockTool); + await core.initialize(); + + core.destroy(); + + expect(error).not.toHaveBeenCalled(); + + error.mockRestore(); + }); + + it('should not log when destroyed before initialize', () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const core = createCore(); + + core.destroy(); + + expect(error).not.toHaveBeenCalled(); + + error.mockRestore(); + }); + + it('should not construct anything when destroyed before initialize', () => { + const created = jest.fn(); + const core = createCore(); + + core.use(createPlugin('a', created) as typeof StubPlugin); + + core.destroy(); + + expect(created).not.toHaveBeenCalled(); + expect(log).toEqual([]); + }); + + it('should reject initialize with an AbortError without dispatching ready when destroyed mid-initialization', async () => { + let resolvePrepare: () => void = () => undefined; + const ready = jest.fn(); + const onModelUpdate = jest.fn(); + const core = createCore({ onModelUpdate }); + + core.use(createPlugin('a', ({ eventBus }) => { + eventBus.addEventListener(`core:${CoreEventType.Ready}`, ready); + }) as typeof StubPlugin); + core.use(class SlowInlineTool { + public static type = ToolType.Inline as const; + public static name = 'slow'; + + public static prepare(): Promise { + return new Promise((resolve) => { + resolvePrepare = resolve; + }); + } + } as unknown as typeof StubBlockTool); + + const initialization = core.initialize(); + + /** + * Let the tools queue reach the pending prepare() + */ + await flush(); + core.destroy(); + resolvePrepare(); + + await expect(initialization).rejects.toMatchObject({ name: 'AbortError', + message: 'Editor was destroyed during initialization' }); + await flush(); + + expect(ready).not.toHaveBeenCalled(); + expect(onModelUpdate).not.toHaveBeenCalled(); + expect(log).toEqual(['plugin:a', 'adapter']); + }); + + it('should throw from use and initialize after destroy', async () => { + const core = createCore(); + + core.destroy(); + + expect(() => core.use(createPlugin('a') as typeof StubPlugin)).toThrow(/destroyed/); + await expect(core.initialize()).rejects.toThrow(/destroyed/); + }); + }); }); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 01ab27ae..22b344ca 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -12,7 +12,9 @@ import { PluginType, ToolType, type ToolStaticOptions, - type PluginId + type PluginId, + type EditorjsPlugin, + type EditorJSAdapterPlugin } from '@editorjs/sdk'; import { composeDataFromVersion2 } from './utils/composeDataFromVersion2.js'; import ToolsManager from './tools/ToolsManager.js'; @@ -31,6 +33,51 @@ import { PluginRegistry } from './components/PluginRegistry.js'; */ const DEFAULT_HOLDER_ID = 'editorjs'; +/** + * Plugin instance created by Core, with the name its public API is registered under + */ +interface PluginInstance { + /** + * Plugin's static name + */ + name: PluginId; + + /** + * Plugin instance + */ + instance: EditorjsPlugin; +} + +/** + * Core services that hold listeners or resources and need teardown + */ +interface CoreServices { + /** + * Tracks the caret and applies inline tools + */ + selectionManager?: SelectionManager; + + /** + * Renders blocks and owns their tool instances + */ + blockRenderer?: BlockRenderer; + + /** + * Local undo/redo history + */ + undoRedoManager?: UndoRedoManager; + + /** + * Rendering adapter plugin + */ + adapter?: EditorJSAdapterPlugin; + + /** + * Owns the tool facades, and through them the shared inline tool instances + */ + toolsManager?: ToolsManager; +} + /** * Editor entry point * - initializes Model @@ -59,6 +106,27 @@ export default class Core { */ #plugins: Container; + /** + * Plugin instances in construction order, destroyed in reverse order + */ + #pluginInstances: PluginInstance[] = []; + + /** + * Core services resolved during initialization, destroyed by {@link destroy}. + * Kept here so teardown never resolves a service that initialization didn't reach + */ + #services: CoreServices = {}; + + /** + * Releases Core's own listeners on destroy + */ + readonly #controller = new AbortController(); + + /** + * Set by {@link destroy}. A destroyed core can't be used or initialized again + */ + #destroyed = false; + /** * @param config - Editor configuration */ @@ -101,7 +169,7 @@ export default class Core { if (config.onModelUpdate !== undefined) { this.#model.addEventListener(EventType.Changed, () => { config.onModelUpdate?.(this.#model); - }); + }, { signal: this.#controller.signal }); } } @@ -129,6 +197,8 @@ export default class Core { pluginOrTool: ToolConstructable | EditorjsPluginConstructor | EditorjsAdapterPluginConstructor, options?: ToolStaticOptions ): Core { + this.#assertNotDestroyed(); + const pluginType = pluginOrTool.type; switch (pluginType) { @@ -155,6 +225,7 @@ export default class Core { * Initializes the core */ public async initialize(): Promise { + this.#assertNotDestroyed(); this.#validatePreconditions(); const { blocks } = composeDataFromVersion2(this.#config.data ?? { blocks: [] }); @@ -167,18 +238,27 @@ export default class Core { * @todo add e2e initialization tests * Currently only BlockRenderer would be enough, but that would be hard to debug. Easier just add every module here */ - this.#iocContainer.get(SelectionManager); + this.#services.selectionManager = this.#iocContainer.get(SelectionManager); this.#iocContainer.get(BlocksManager); - this.#iocContainer.get(BlockRenderer); + this.#services.blockRenderer = this.#iocContainer.get(BlockRenderer); + this.#services.adapter = this.#iocContainer.get(TOKENS.Adapter); + this.#services.toolsManager = this.#iocContainer.get(ToolsManager); this.#initializePlugins(); await this.#initializeTools(); + /** + * destroy() was called while tools were being prepared: it already tore down what exists + */ + if (this.#destroyed) { + throw new DOMException('Editor was destroyed during initialization', 'AbortError'); + } + /** * UndoRedoManager should go after plugins as it uses defaultPrevented on undo/redo events which can be set by plugins * @todo Figure out how to make initialization less complex */ - this.#iocContainer.get(UndoRedoManager); + this.#services.undoRedoManager = this.#iocContainer.get(UndoRedoManager); this.#model.initializeDocument({ blocks }); @@ -187,6 +267,64 @@ export default class Core { eventBus.dispatchEvent(new CoreEventBase(CoreEventType.Ready, undefined)); } + /** + * Tears the editor down: plugins in reverse construction order, core services, rendered blocks, and the adapter. + * The document model is left untouched, so no removal reaches undo history or collaboration. + * Safe to call more than once and while {@link initialize} is pending. After it, `use()` and `initialize()` throw + */ + public destroy(): void { + /** + * Plugin instances and services are cleared below, so a second call finds nothing left to destroy + */ + this.#destroyed = true; + this.#controller.abort(); + + const registry = this.#iocContainer.get(PluginRegistry); + + for (const { name, instance } of this.#pluginInstances.reverse()) { + this.#safely(`plugin "${name}"`, () => instance.destroy?.()); + registry.unregister(name); + } + + this.#pluginInstances = []; + + const { undoRedoManager, selectionManager, blockRenderer, adapter, toolsManager } = this.#services; + + this.#safely('UndoRedoManager', () => undoRedoManager?.destroy()); + this.#safely('SelectionManager', () => selectionManager?.destroy()); + this.#safely('BlockRenderer', () => blockRenderer?.destroy()); + this.#safely('adapter', () => adapter?.destroy?.()); + + /** + * Last: the shared inline tool instances are used by the adapter and the UI until they're gone + */ + this.#safely('ToolsManager', () => toolsManager?.destroy()); + + this.#services = {}; + } + + /** + * Runs one teardown step, logging its error so the remaining steps still run + * @param label - what is being destroyed, for the log + * @param step - teardown step + */ + #safely(label: string, step: () => void): void { + try { + step(); + } catch (error) { + console.error(`[Core] Failed to destroy ${label}`, error); + } + } + + /** + * Throws if {@link destroy} has been called + */ + #assertNotDestroyed(): void { + if (this.#destroyed) { + throw new Error('Editor has been destroyed. Create a new instance instead'); + } + } + /** * Validates that the caller has registered everything the headless engine needs before booting. * Throws with a clear message instead of failing deep inside module resolution. @@ -252,6 +390,9 @@ export default class Core { eventBus, }); + this.#pluginInstances.push({ name: plugin.name, + instance }); + if (instance.publicApi !== undefined) { this.#iocContainer.get(PluginRegistry).register(plugin.name, instance.publicApi); } diff --git a/packages/core/src/tools/ToolsManager.spec.ts b/packages/core/src/tools/ToolsManager.spec.ts new file mode 100644 index 00000000..17ed2116 --- /dev/null +++ b/packages/core/src/tools/ToolsManager.spec.ts @@ -0,0 +1,288 @@ +import { describe, expect, it, jest } from '@jest/globals'; +import type { CoreConfigValidated, InlineToolConstructor } from '@editorjs/sdk'; +import { EventBus, ToolType } from '@editorjs/sdk'; +import ToolsManager from './ToolsManager.js'; +import type { EditorAPI } from '../api/index.js'; + +/** + * Inline tool whose static prepare() settles only when the test calls finish() + */ +interface PendingTool { + /** + * The tool class + */ + tool: InlineToolConstructor; + + /** + * Its static prepare() + */ + prepare: jest.Mock; + + /** + * Settles the pending prepare() + */ + finish: () => void; +} + +describe('ToolsManager', () => { + describe('.prepareTools()', () => { + it('should not destroy the shared inline tool instance it validates', async () => { + const destroy = jest.fn(); + + /** + * Inline tool with a static prepare(), so the validation branch runs + */ + class ProbeInlineTool { + public static type = ToolType.Inline; + + public static name = 'probe'; + + public static prepare = jest.fn(() => Promise.resolve()); + + public destroy = destroy; + } + + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + new EventBus() + ); + + await manager.prepareTools([[ProbeInlineTool as unknown as InlineToolConstructor, undefined]]); + + expect(destroy).not.toHaveBeenCalled(); + }); + + /** + * Prepares a single inline tool whose instance is built by `create` + * @param instance - prototype members of the tool instance + */ + const prepareInlineTool = async (instance: Record): Promise => { + /** + * Inline tool whose instance carries the given members + */ + class ProbeInlineTool { + public static type = ToolType.Inline; + + public static name = 'probe'; + + /** + * Copies the given members onto the instance + */ + constructor() { + Object.assign(this, instance); + } + + /** + * Static prepare, so the validation branch runs + */ + public static prepare(): Promise { + return Promise.resolve(); + } + } + + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + new EventBus() + ); + + await manager.prepareTools([[ProbeInlineTool as unknown as InlineToolConstructor, undefined]]); + + return manager; + }; + + it('should make an inline tool without render available', async () => { + const manager = await prepareInlineTool({}); + + expect(manager.inlineTools.get('probe')).toBeDefined(); + expect(manager.unavailable.get('probe')).toBeUndefined(); + }); + + it('should mark an inline tool implementing the v2 render method as unavailable', async () => { + const log = jest.spyOn(console, 'log').mockImplementation(() => undefined); + const manager = await prepareInlineTool({ render: () => undefined }); + + expect(manager.inlineTools.get('probe')).toBeUndefined(); + expect(manager.unavailable.get('probe')).toBeDefined(); + + log.mockRestore(); + }); + }); + + describe('.prepareTools() interrupted by .destroy()', () => { + /** + * Inline tool class with a static prepare() that settles when the test says so + * @param name - static name the tool is registered under + * @param constructed - called by the tool's constructor + */ + const pendingTool = (name: string, constructed: () => void = jest.fn()): PendingTool => { + let finish: () => void = () => undefined; + const prepare = jest.fn(() => new Promise((resolve) => { + finish = resolve; + })); + + /** + * Inline tool whose preparation the test controls + */ + class PendingInlineTool { + public static type = ToolType.Inline; + + public static name = name; + + public static prepare = prepare; + + /** + * Reports its construction + */ + constructor() { + constructed(); + } + } + + return { + tool: PendingInlineTool as unknown as InlineToolConstructor, + prepare, + finish: () => finish(), + }; + }; + + /** + * Lets queued promise jobs run + */ + const flush = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + + it('should not prepare the tools still queued', async () => { + const first = pendingTool('first'); + const second = pendingTool('second'); + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + new EventBus() + ); + + const preparing = manager.prepareTools([[first.tool, undefined], [second.tool, undefined]]); + + await flush(); + manager.destroy(); + first.finish(); + await preparing; + + expect(first.prepare).toHaveBeenCalledTimes(1); + expect(second.prepare).not.toHaveBeenCalled(); + }); + + it('should neither create nor announce a tool whose prepare() settles after destroy', async () => { + const constructed = jest.fn(); + const pending = pendingTool('late', constructed); + const eventBus = new EventBus(); + const dispatch = jest.spyOn(eventBus, 'dispatchEvent'); + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + eventBus + ); + + const preparing = manager.prepareTools([[pending.tool, undefined]]); + + await flush(); + manager.destroy(); + pending.finish(); + await preparing; + + expect(pending.prepare).toHaveBeenCalledTimes(1); + expect(constructed).not.toHaveBeenCalled(); + expect(dispatch).not.toHaveBeenCalled(); + expect(manager.available.get('late')).toBeUndefined(); + expect(manager.unavailable.get('late')).toBeUndefined(); + }); + }); + + describe('.destroy()', () => { + /** + * Inline tool class whose instances call `onDestroy` from their destroy() hook + * @param name - static name the tool is registered under + * @param onDestroy - called by the instance's destroy() + * @param withPrepare - give the tool a static prepare(), so prepareTools validates it + */ + const inlineTool = (name: string, onDestroy: () => void, withPrepare = false): InlineToolConstructor => { + /** + * Inline tool that reports when its instance is destroyed + */ + class ProbeInlineTool { + public static type = ToolType.Inline; + + public static name = name; + + /** + * Destroy hook under test + */ + public destroy(): void { + onDestroy(); + } + } + + if (withPrepare) { + Object.assign(ProbeInlineTool, { prepare: (): Promise => Promise.resolve() }); + } + + return ProbeInlineTool as unknown as InlineToolConstructor; + }; + + /** + * Creates a manager with the given tools prepared + * @param tools - inline tool classes to prepare + */ + const prepared = async (...tools: InlineToolConstructor[]): Promise => { + const manager = new ToolsManager( + { tools: {} } as unknown as CoreConfigValidated, + () => ({}) as EditorAPI, + new EventBus() + ); + + await manager.prepareTools(tools.map(tool => [tool, undefined])); + + return manager; + }; + + it('should destroy each inline tool instance once', async () => { + const destroyA = jest.fn(); + const destroyB = jest.fn(); + const manager = await prepared(inlineTool('a', destroyA), inlineTool('b', destroyB, true)); + + manager.inlineTools.get('a')?.create(); + + manager.destroy(); + manager.destroy(); + + expect(destroyA).toHaveBeenCalledTimes(1); + expect(destroyB).toHaveBeenCalledTimes(1); + }); + + it('should not create an instance for a tool that was never used', async () => { + const destroy = jest.fn(); + const manager = await prepared(inlineTool('unused', destroy)); + + manager.destroy(); + + expect(destroy).not.toHaveBeenCalled(); + }); + + it('should log a throwing destroy() and still destroy the other tools', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + const destroyB = jest.fn(); + const manager = await prepared( + inlineTool('a', () => { + throw new Error('broken tool'); + }, true), + inlineTool('b', destroyB, true) + ); + + expect(() => manager.destroy()).not.toThrow(); + expect(destroyB).toHaveBeenCalledTimes(1); + expect(error).toHaveBeenCalledWith('[ToolsManager] Inline tool a failed to destroy', expect.any(Error)); + + error.mockRestore(); + }); + }); +}); diff --git a/packages/core/src/tools/ToolsManager.ts b/packages/core/src/tools/ToolsManager.ts index 297f30fd..af2244d7 100644 --- a/packages/core/src/tools/ToolsManager.ts +++ b/packages/core/src/tools/ToolsManager.ts @@ -56,6 +56,12 @@ export default class ToolsManager { */ #unavailableTools = new ToolsCollection(); + /** + * Aborted by {@link destroy}: tools still queued for preparation are skipped, and a tool whose + * prepare() settles afterwards is neither created nor registered + */ + readonly #controller = new AbortController(); + /** * Returns available Tools */ @@ -110,12 +116,40 @@ export default class ToolsManager { this.#validateTools(); } + /** + * Destroys the shared inline tool instances, once each. + * + * Every inline tool facade hands the same instance to all its consumers (formatting, selection, + * the inline toolbar), so none of them destroys it: the facade owns it and is destroyed here, on + * editor teardown. A throwing hook is logged and the remaining tools are still destroyed. + * + * Also stops a {@link prepareTools} still running: no tool is prepared, created or announced after this + */ + public destroy(): void { + this.#controller.abort(); + + const facades = new Set([...this.#availableTools.values(), ...this.#unavailableTools.values()]); + + for (const facade of facades) { + if (!facade.isInline()) { + continue; + } + + try { + facade.destroy(); + } catch (error) { + console.error(`[ToolsManager] Inline tool ${facade.name} failed to destroy`, error); + } + } + } + /** * Calls tools prepare method if it exists and adds tools to relevant collection (available or unavailable tools) * @param tools - tools to prepare and their settings */ public async prepareTools(tools: [ToolConstructable, ToolStaticOptions | undefined][]): Promise { const promiseQueue = new PromiseQueue(); + const { signal } = this.#controller; const setToAvailableToolsCollection = (toolName: string, tool: ToolFacadeClass): void => { this.#availableTools.set(toolName, tool); @@ -134,6 +168,13 @@ export default class ToolsManager { if (isFunction(toolConstructor.prepare)) { void promiseQueue.add(async () => { + /** + * The editor was destroyed while earlier tools were being prepared + */ + if (signal.aborted) { + return; + } + try { const tool = factory.get(toolName); @@ -145,12 +186,24 @@ export default class ToolsManager { config: tool.config, }); + /** + * The editor was destroyed while this tool was preparing. An inline instance created now + * would outlive destroy(), and ToolLoaded would reach a torn-down editor + */ + if (signal.aborted) { + return; + } + if (tool.isInline()) { /** * Some Tools validation */ const inlineToolRequiredMethods = ['render']; - const notImplementedMethods = inlineToolRequiredMethods.filter(method => tool.create()[method as keyof InlineTool] !== undefined); + /** + * The facade's shared instance: it stays alive for the other consumers and is destroyed on teardown + */ + const instance = tool.create(); + const notImplementedMethods = inlineToolRequiredMethods.filter(method => instance[method as keyof InlineTool] !== undefined); if (notImplementedMethods.length > 0) { /** diff --git a/packages/editorjs/src/index.spec.ts b/packages/editorjs/src/index.spec.ts index 5e3166e4..201c6bec 100644 --- a/packages/editorjs/src/index.spec.ts +++ b/packages/editorjs/src/index.spec.ts @@ -9,6 +9,7 @@ import { jest, describe, it, expect, beforeEach } from '@jest/globals'; */ const use = jest.fn(); const initialize = jest.fn<() => Promise>(); +const destroy = jest.fn(); /** * Minimal Core stand-in recording `use()` calls and delegating `initialize()`. @@ -16,6 +17,7 @@ const initialize = jest.fn<() => Promise>(); class MockCore { public use = use; public initialize = initialize; + public destroy = destroy; } jest.unstable_mockModule('@editorjs/core', () => ({ default: MockCore })); @@ -40,6 +42,40 @@ describe('EditorJS bundle', () => { beforeEach(() => { use.mockClear(); initialize.mockReset(); + destroy.mockClear(); + }); + + describe('isReady interrupted by destroy', () => { + /** + * Lets an unhandled rejection surface: Jest fails the test that leaves one behind + */ + const flush = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + + it('should not leave an AbortError from isReady unhandled', async () => { + initialize.mockRejectedValue(new DOMException('Editor was destroyed during initialization', 'AbortError')); + + void new EditorJS({} as any); + + await expect(flush()).resolves.toBeUndefined(); + }); + + it('should still reject isReady with the AbortError', async () => { + initialize.mockRejectedValue(new DOMException('Editor was destroyed during initialization', 'AbortError')); + + const editor = new EditorJS({} as any); + + await expect(editor.isReady).rejects.toMatchObject({ name: 'AbortError' }); + }); + }); + + it('should delegate destroy to Core', () => { + initialize.mockResolvedValue(undefined); + + const editor = new EditorJS({} as any); + + editor.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); }); it('exposes isReady that resolves when Core initialization completes', async () => { @@ -52,11 +88,16 @@ describe('EditorJS bundle', () => { }); it('rejects isReady when Core initialization fails', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + initialize.mockRejectedValue(new Error('init failed')); const editor = new EditorJS({} as any); await expect(editor.isReady).rejects.toThrow('init failed'); + expect(error).toHaveBeenCalledWith('[EditorJS] Initialization failed', expect.any(Error)); + + error.mockRestore(); }); it('registers a rendering adapter on the underlying Core', () => { diff --git a/packages/editorjs/src/index.ts b/packages/editorjs/src/index.ts index 67281ee2..8f382485 100644 --- a/packages/editorjs/src/index.ts +++ b/packages/editorjs/src/index.ts @@ -11,6 +11,7 @@ import { ClipboardPlugin } from '@editorjs/clipboard-plugin'; import { ShortcutsPlugin } from '@editorjs/shortcuts-plugin'; import { EditorjsUI, BlocksUI, InlineToolbarUI, ToolbarUI, ToolboxUI } from '@editorjs/ui'; import { mergeTools } from './mergeTools.js'; +import { reportUnlessAborted } from './reportUnlessAborted.js'; /** * Default tools registered by the bundle, keyed later by their static `name`. @@ -94,14 +95,28 @@ export default class EditorJS { this.#core.use(ToolboxUI); this.#readyPromise = this.#core.initialize(); + + /** + * `isReady` may be left unawaited: ignore the AbortError a destroy before ready causes, log anything else + */ + reportUnlessAborted(this.#readyPromise); } /** - * Resolves once the editor has finished initializing; rejects if initialization fails. + * Resolves once the editor has finished initializing. Rejects if initialization fails, + * or with an `AbortError` if the editor is destroyed before it is ready */ public get isReady(): Promise { return this.#readyPromise; } + + /** + * Tears the editor down: removes its DOM, closes the collaboration connection, and releases plugins and tools. + * Safe to call more than once and before {@link isReady} settles + */ + public destroy(): void { + this.#core.destroy(); + } } export { Core }; diff --git a/packages/editorjs/src/reportUnlessAborted.spec.ts b/packages/editorjs/src/reportUnlessAborted.spec.ts new file mode 100644 index 00000000..9f23566e --- /dev/null +++ b/packages/editorjs/src/reportUnlessAborted.spec.ts @@ -0,0 +1,51 @@ +import { afterEach, beforeEach, describe, expect, it, jest } from '@jest/globals'; +import { reportUnlessAborted } from './reportUnlessAborted.js'; + +/** + * Lets the attached handler run + */ +const flush = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + +describe('reportUnlessAborted', () => { + let error: jest.SpiedFunction; + + beforeEach(() => { + error = jest.spyOn(console, 'error').mockImplementation(() => undefined); + }); + + afterEach(() => { + error.mockRestore(); + }); + + it('should ignore an AbortError rejection', async () => { + reportUnlessAborted(Promise.reject(new DOMException('aborted', 'AbortError'))); + await flush(); + + expect(error).not.toHaveBeenCalled(); + }); + + it('should log any other rejection', async () => { + const failure = new Error('init failed'); + + reportUnlessAborted(Promise.reject(failure)); + await flush(); + + expect(error).toHaveBeenCalledWith('[EditorJS] Initialization failed', failure); + }); + + it('should log a DOMException of another name', async () => { + const failure = new DOMException('bad state', 'InvalidStateError'); + + reportUnlessAborted(Promise.reject(failure)); + await flush(); + + expect(error).toHaveBeenCalledWith('[EditorJS] Initialization failed', failure); + }); + + it('should not log when the promise resolves', async () => { + reportUnlessAborted(Promise.resolve()); + await flush(); + + expect(error).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/editorjs/src/reportUnlessAborted.ts b/packages/editorjs/src/reportUnlessAborted.ts new file mode 100644 index 00000000..b568db7b --- /dev/null +++ b/packages/editorjs/src/reportUnlessAborted.ts @@ -0,0 +1,17 @@ +/** + * Handles the bundle's own ready promise so its rejection is never reported as unhandled. + * + * Destroying the editor before it is ready rejects `isReady` with an `AbortError`: that is expected + * and ignored. Any other initialization error is logged, so it isn't lost when nobody awaits `isReady`. + * Callers awaiting `isReady` still get the rejection, since this only attaches a handler. + * @param promise - the ready promise + */ +export function reportUnlessAborted(promise: Promise): void { + promise.catch((error: unknown) => { + if (error instanceof DOMException && error.name === 'AbortError') { + return; + } + + console.error('[EditorJS] Initialization failed', error); + }); +} diff --git a/packages/model/src/EditorJSModel.ts b/packages/model/src/EditorJSModel.ts index 8b9d7521..3682e415 100644 --- a/packages/model/src/EditorJSModel.ts +++ b/packages/model/src/EditorJSModel.ts @@ -36,12 +36,12 @@ export interface EditorJSModel { /** * Overload for EditorDocument events */ - addEventListener(type: EventType.Changed, listener: (event: K) => void): void; + addEventListener(type: EventType.Changed, listener: (event: K) => void, options?: AddEventListenerOptions | boolean): void; /** * Overload for CaretManager events */ - addEventListener(type: EventType.CaretManagerUpdated, listener: (event: K) => void): void; + addEventListener(type: EventType.CaretManagerUpdated, listener: (event: K) => void, options?: AddEventListenerOptions | boolean): void; } /** diff --git a/packages/sdk/src/entities/BlockTool.ts b/packages/sdk/src/entities/BlockTool.ts index 1f77f69c..a696f97c 100644 --- a/packages/sdk/src/entities/BlockTool.ts +++ b/packages/sdk/src/entities/BlockTool.ts @@ -163,7 +163,13 @@ export type BlockTool< */ // eslint-disable-next-line @typescript-eslint/no-unused-vars, @typescript-eslint/no-explicit-any Config extends ToolConfig = any -> = Omit; +> = Omit & { + /** + * Releases resources held by the instance. + * Called once, when the block is removed or the editor is destroyed + */ + destroy?(): void; +}; /** * Block Tool constructor class diff --git a/packages/sdk/src/entities/BlockToolAdapter.spec.ts b/packages/sdk/src/entities/BlockToolAdapter.spec.ts new file mode 100644 index 00000000..78c2465a --- /dev/null +++ b/packages/sdk/src/entities/BlockToolAdapter.spec.ts @@ -0,0 +1,114 @@ +/* eslint-disable jsdoc/require-jsdoc */ + +import { describe, expect, it, jest } from '@jest/globals'; +import type { CoreConfig } from './Config.js'; +import type { EditorAPI } from '../api/index.js'; +import type { BlockId, EventBus, ModelEvents } from '@editorjs/model-types'; +import { BlockToolAdapter } from './BlockToolAdapter.js'; + +const MODEL_EVENT = 'model:update'; +const BUS_EVENT = 'ui:probe'; + +/** + * Adapter subclass that registers its own listener with the inherited signal + */ +class ProbeAdapter extends BlockToolAdapter { + public readonly modelUpdates = jest.fn(); + + public readonly busEvents = jest.fn(); + + constructor(config: Required, api: EditorAPI, eventBus: EventBus) { + super(config, api, eventBus); + + eventBus.addEventListener(BUS_EVENT, () => this.busEvents(), { signal: this.signal }); + } + + protected handleModelUpdate(event: ModelEvents): void { + this.modelUpdates(event); + } +} + +/** + * Adapter under test with the targets standing in for the model and the EventBus + */ +interface Harness { + adapter: ProbeAdapter; + model: EventTarget; + eventBus: EventTarget; +} + +/** + * Creates an adapter wired to real EventTargets standing in for the model and the EventBus + */ +function createAdapter(): Harness { + const model = new EventTarget(); + const eventBus = new EventTarget(); + const blockId = 'block' as BlockId; + + const api = { + document: { + onUpdate: (callback: EventListener) => { + const controller = new AbortController(); + + model.addEventListener(MODEL_EVENT, callback, { signal: controller.signal }); + + return () => controller.abort(); + }, + }, + blocks: { + getIdByIndex: () => blockId, + }, + } as unknown as EditorAPI; + + const adapter = new ProbeAdapter({} as Required, api, eventBus as EventBus); + + adapter.setBlockId(blockId); + + return { adapter, + model, + eventBus }; +} + +/** + * Model event shaped enough for the adapter's index check + */ +function modelEvent(): Event { + return Object.assign(new Event(MODEL_EVENT), { detail: { index: { blockIndex: 0 } } }); +} + +describe('BlockToolAdapter', () => { + it('should handle model updates before destroy', () => { + const { adapter, model } = createAdapter(); + + model.dispatchEvent(modelEvent()); + + expect(adapter.modelUpdates).toHaveBeenCalledTimes(1); + }); + + it('should stop handling model updates after destroy', () => { + const { adapter, model } = createAdapter(); + + adapter.destroy(); + model.dispatchEvent(modelEvent()); + + expect(adapter.modelUpdates).not.toHaveBeenCalled(); + }); + + it('should remove a subclass listener registered with the inherited signal on destroy', () => { + const { adapter, eventBus } = createAdapter(); + + eventBus.dispatchEvent(new Event(BUS_EVENT)); + adapter.destroy(); + eventBus.dispatchEvent(new Event(BUS_EVENT)); + + expect(adapter.busEvents).toHaveBeenCalledTimes(1); + }); + + it('should abort the inherited signal on destroy', () => { + const { adapter } = createAdapter(); + + adapter.destroy(); + + expect((adapter as unknown as { signal: AbortSignal }).signal.aborted).toBe(true); + }); +}); diff --git a/packages/sdk/src/entities/BlockToolAdapter.ts b/packages/sdk/src/entities/BlockToolAdapter.ts index faa6d212..0d95045f 100644 --- a/packages/sdk/src/entities/BlockToolAdapter.ts +++ b/packages/sdk/src/entities/BlockToolAdapter.ts @@ -38,9 +38,15 @@ export abstract class BlockToolAdapter extends EventTarget { protected eventBus: EventBus; /** - * Stored reference to the model change listener so it can be removed on destroy. + * Releases every listener registered by the adapter and its subclasses on destroy */ - #modelChangeListenerCleanup: () => void; + readonly #controller = new AbortController(); + + /** + * Signal subclasses pass to their own listener registrations. + * It is aborted by {@link destroy}, so subclasses don't need to remove listeners themselves + */ + protected readonly signal: AbortSignal = this.#controller.signal; /** * @param config - editor's configuration @@ -54,19 +60,20 @@ export abstract class BlockToolAdapter extends EventTarget { this.config = config; this.eventBus = eventBus; - this.#modelChangeListenerCleanup = this.#api.document.onUpdate( + const unsubscribe = this.#api.document.onUpdate( ((event: ModelEvents) => this.#handleModelUpdate(event)) as EventListener ); + + this.signal.addEventListener('abort', unsubscribe); } /** * Releases all resources held by this adapter. - * Removes the model change listener registered in the constructor. - * Subclasses that register additional listeners should override this method, - * call `super.destroy()`, and then remove their own listeners. + * Aborts {@link signal}, removing the model listener and every listener subclasses registered with it. + * Subclasses override this method only to release resources that aren't listeners, and call `super.destroy()` */ public destroy(): void { - this.#modelChangeListenerCleanup(); + this.#controller.abort(); } /** diff --git a/packages/sdk/src/entities/InlineTool.ts b/packages/sdk/src/entities/InlineTool.ts index 3e167e87..da7be561 100644 --- a/packages/sdk/src/entities/InlineTool.ts +++ b/packages/sdk/src/entities/InlineTool.ts @@ -125,6 +125,12 @@ export interface InlineTool extends Omit; + + /** + * Releases resources held by the instance. + * Called once, when the editor stops using this instance + */ + destroy?(): void; } /** diff --git a/packages/sdk/src/entities/ToolDestroy.spec.ts b/packages/sdk/src/entities/ToolDestroy.spec.ts new file mode 100644 index 00000000..cade0252 --- /dev/null +++ b/packages/sdk/src/entities/ToolDestroy.spec.ts @@ -0,0 +1,56 @@ +import { describe, expect, it, jest } from '@jest/globals'; +import type { BlockTool } from './BlockTool.js'; +import type { InlineTool } from './InlineTool.js'; + +/** + * Builds the minimal inline tool members, so each case only adds what it checks + */ +function inlineToolBase(): Omit { + return { + isActive: () => false, + getFormattingOptions: () => ({ action: 'format', + range: [0, 0] }) as never, + createWrapper: () => ({}) as HTMLElement, + getToolbarConfig: () => ({}) as never, + }; +} + +describe('Tool destroy contract', () => { + it('should accept an inline tool that implements destroy', () => { + const destroy = jest.fn(); + const tool: InlineTool = { + ...inlineToolBase(), + destroy, + }; + + tool.destroy?.(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('should accept an inline tool without destroy', () => { + const tool: InlineTool = inlineToolBase(); + + expect('destroy' in tool).toBe(false); + }); + + it('should accept a block tool that implements destroy', () => { + const destroy = jest.fn(); + const tool: BlockTool = { + render: () => ({}) as HTMLElement, + destroy, + }; + + tool.destroy?.(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('should accept a block tool without destroy', () => { + const tool: BlockTool = { + render: () => ({}) as HTMLElement, + }; + + expect('destroy' in tool).toBe(false); + }); +}); diff --git a/packages/sdk/src/tools/facades/InlineToolFacade.spec.ts b/packages/sdk/src/tools/facades/InlineToolFacade.spec.ts new file mode 100644 index 00000000..2c07b347 --- /dev/null +++ b/packages/sdk/src/tools/facades/InlineToolFacade.spec.ts @@ -0,0 +1,96 @@ +/* eslint-disable jsdoc/require-jsdoc */ + +import { describe, expect, it, jest } from '@jest/globals'; +import type { InlineToolConstructor } from '../../entities/index.js'; +import { ToolType } from '../../entities/index.js'; +import type { ToolOptions } from './BaseToolFacade.js'; +import { InlineToolFacade } from './InlineToolFacade.js'; +import type { EditorAPI } from '../../api'; + +/** + * Facade under test, with a record of how many instances its tool class constructed + */ +interface Probe { + facade: InlineToolFacade; + constructed: jest.Mock; +} + +/** + * Creates a facade around an inline tool class whose instances record their construction and destruction + * @param destroy - the instances' destroy() hook, or undefined for a tool without one + */ +function createFacade(destroy?: () => void): Probe { + const constructed = jest.fn(); + + class MockInlineTool { + public static type = ToolType.Inline; + + public destroy = destroy; + + constructor() { + constructed(); + } + } + + const facade = new InlineToolFacade({ + api: {} as EditorAPI, + constructable: MockInlineTool as unknown as InlineToolConstructor, + isDefault: false, + name: 'probe', + useToolOptions: {} as ToolOptions, + }); + + return { facade, + constructed }; +} + +describe('InlineToolFacade', () => { + it('should return the same instance from every create()', () => { + const { facade, constructed } = createFacade(); + + expect(facade.create()).toBe(facade.create()); + expect(constructed).toHaveBeenCalledTimes(1); + }); + + it('should call the instance\'s destroy() once on destroy', () => { + const destroy = jest.fn(); + const { facade } = createFacade(destroy); + + facade.create(); + facade.destroy(); + facade.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it('should not construct an instance on destroy when none was created', () => { + const { facade, constructed } = createFacade(jest.fn()); + + facade.destroy(); + + expect(constructed).not.toHaveBeenCalled(); + }); + + it('should support a tool without destroy()', () => { + const { facade } = createFacade(); + + facade.create(); + + expect(() => facade.destroy()).not.toThrow(); + }); + + it('should drop the instance even when its destroy() throws', () => { + const destroy = jest.fn(() => { + throw new Error('broken tool'); + }); + const { facade } = createFacade(destroy); + + facade.create(); + + expect(() => facade.destroy()).toThrow('broken tool'); + + facade.destroy(); + + expect(destroy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/sdk/src/tools/facades/InlineToolFacade.ts b/packages/sdk/src/tools/facades/InlineToolFacade.ts index a1faeab4..0d57b4a0 100644 --- a/packages/sdk/src/tools/facades/InlineToolFacade.ts +++ b/packages/sdk/src/tools/facades/InlineToolFacade.ts @@ -38,7 +38,8 @@ export class InlineToolFacade extends BaseToolFacade