diff --git a/docs/README.md b/docs/README.md index 8fb98390..37de0c3b 100644 --- a/docs/README.md +++ b/docs/README.md @@ -39,6 +39,7 @@ Two event transports (never mixed): - Resolves core services (`SelectionManager`, `BlocksManager`, `BlockRenderer`, `UndoRedoManager`). - Inserts initial document into `EditorJSModel`; `BlockRenderer` reacts to each `BlockAddedEvent` to create a `BlockToolAdapter`, render the tool, and emit `BlockAddedCoreEvent`. - Dispatches `CoreEventType.Ready`; collaboration manager connects if server config is provided. +4. `core.destroy()` tears it down: plugins in reverse construction order (UI removes its DOM, collaboration closes its socket), then `UndoRedoManager`, `SelectionManager`, `BlockRenderer` (destroying each block's tool instance and adapter), the adapter plugin, and last the shared inline tool instances, once each. The model is left untouched; the holder element stays in place. --- diff --git a/docs/diagrams/plugin-lifecycle-flow.mmd b/docs/diagrams/plugin-lifecycle-flow.mmd index 7c9d9250..00629d60 100644 --- a/docs/diagrams/plugin-lifecycle-flow.mmd +++ b/docs/diagrams/plugin-lifecycle-flow.mmd @@ -43,7 +43,7 @@ sequenceDiagram Core->>Plugin: new SomePlugin(config, api, eventBus) Plugin->>EventBus: wire event listeners Core->>Registry: register(SomePlugin.name, plugin.publicApi) (if exposed) - Note over Registry: shared record behind api.plugins;
read at access time, so plugins constructed
earlier still see later registrations + Note over Registry: shared record behind api.plugins,
read at access time, so plugins constructed
earlier still see later registrations Core->>CollabMgr: new CollaborationManager(config, api, eventBus) CollabMgr->>EventBus: wire event listeners @@ -68,6 +68,22 @@ sequenceDiagram CollabMgr->>CollabMgr: connect() (if server config provided) %% ── Destroy ────────────────────────────────────── - Dev->>Plugin: plugin.destroy() - Plugin->>EventBus: removeEventListener (cleanup) - Note over Registry: the registry lives for the lifetime of the Core
instance — Core does not retain plugin instances,
so nothing calls unregister() on teardown today + Dev->>Core: destroy() + loop plugins, in reverse construction order + Core->>Plugin: plugin.destroy?.() + Plugin->>EventBus: abort() its listeners' signal + Core->>Registry: unregister(plugin.name) + end + Core->>CollabMgr: destroy() (as a plugin: aborts listeners, closes OTClient socket) + Core->>IoC: UndoRedoManager.destroy(), SelectionManager.destroy() + Core->>BlockRenderer: destroy() + loop each rendered block + BlockRenderer->>Tool: instance.destroy?.() + BlockRenderer->>Adapter: destroyBlockToolAdapter(blockId) + end + Core->>Adapter: destroy() + Core->>ToolsManager: destroy() + loop each inline tool facade + ToolsManager->>Tool: facade.destroy(), the shared instance's destroy?.() once + end + Note over EditorJSModel: never mutated during teardown,
so no removals reach undo history or collaboration diff --git a/docs/plugins.md b/docs/plugins.md index 55156296..e0d4d076 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -42,7 +42,10 @@ Canonical startup order: ## Lifecycle boundary - Plugins receive dependencies via constructor params (`config`, `api`, `eventBus`). -- Plugin instances may implement `destroy()`, but `Core` currently does not expose a global `destroy()` lifecycle hook. UI plugins abort their own EventBus/DOM subscriptions before removing their holders and disposing their popovers. Pending inline-toolbar configuration cannot recreate UI after destruction; other instances sharing the bus remain active. +- `core.destroy()` (or `editor.destroy()` on the `@editorjs/editorjs` bundle) tears the editor down synchronously. It calls each plugin's optional `destroy()` in reverse construction order and removes its `api.plugins` entry, then destroys core services, every rendered block (the tool instance's optional `destroy()`, then its adapter), the adapter plugin's `destroy()`, and finally each inline tool's shared instance. A throwing `destroy()` is logged and teardown continues. The document model is never mutated, so teardown sends nothing to undo history or collaboration. Calling it twice is a no-op, calling it while `initialize()` is pending makes `initialize()` reject with an `AbortError` without dispatching `core:ready`, and `use()`/`initialize()` throw afterwards. +- Release listeners through one `AbortController` per component: pass its `signal` to every `addEventListener` (EventBus, DOM) and tie the unsubscribe functions returned by `api.document.onUpdate`/`api.selection.onCaretUpdate` to its `abort` event (`signal.addEventListener('abort', api.document.onUpdate(cb))`). `destroy()` then calls `abort()` and releases whatever isn't a listener (timers, sockets, DOM nodes). `BlockToolAdapter` subclasses use the inherited `protected signal` instead of overriding `destroy()`. +- Block and inline tools may implement an optional `destroy()`, and the editor calls it once per instance. A block tool gets a new instance per block, destroyed when the block is removed or the editor is destroyed. An inline tool has one instance, shared by formatting, the inline toolbar and selection, so it is destroyed once, when the editor is destroyed. It is not destroyed when the toolbar is rebuilt or hidden. +- UI plugins abort their own EventBus/DOM subscriptions before removing their holders and disposing their popovers. Pending inline-toolbar configuration cannot recreate UI after destruction; other instances sharing the bus remain active. - Plugins are constructed in registration order, and `api.plugins` is populated as each one is constructed. **Reading another plugin's API inside your constructor returns `undefined`** — do it after `core:ready`, or lazily inside an event handler. `api.plugins` resolves entries at access time, so a plugin constructed first still sees one registered later. ## Keyboard input diff --git a/openspec/changes/editor-destroy/.openspec.yaml b/openspec/changes/archive/2026-10-16-editor-destroy/.openspec.yaml similarity index 100% rename from openspec/changes/editor-destroy/.openspec.yaml rename to openspec/changes/archive/2026-10-16-editor-destroy/.openspec.yaml diff --git a/openspec/changes/editor-destroy/design.md b/openspec/changes/archive/2026-10-16-editor-destroy/design.md similarity index 95% rename from openspec/changes/editor-destroy/design.md rename to openspec/changes/archive/2026-10-16-editor-destroy/design.md index 8692f0f8..609ff47d 100644 --- a/openspec/changes/editor-destroy/design.md +++ b/openspec/changes/archive/2026-10-16-editor-destroy/design.md @@ -6,9 +6,9 @@ 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 | +| `EditorjsUI` | wrapper `div` appended to the integrator's holder | fixed by #174 | +| `BlocksUI` | blocks holder DOM listeners (`beforeinput`, `keydown`, `copy`), block wrappers | fixed by #174 | +| `ToolbarUI` / `ToolboxUI` / `InlineToolbarUI` | EventBus listeners, popovers | fixed by #174 | | `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 | @@ -126,7 +126,8 @@ Plugins and tools are interfaces, not base classes, so they own their own contro - [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. +- [#174 overlapped with UI teardown] → #174 (merged) covers every UI plugin's teardown, so this change has no UI plugin teardown of its own. `InlineToolbarUI` doesn't destroy inline tool instances either, because they're shared (D7). +- [ui-kit's tooltip singleton kept a `window` `scroll` listener and its element] → it was created lazily and never destroyed, so a page that showed (or merely tore down) a hint kept both after every editor was gone. Fixed upstream: `codex-tooltip` 1.1.0 (codex-team/codex.tooltips#32) only holds them while a hint is shown, and `@editorjs/ui-kit` 2.0.1 (editor-js/utils#43) bundles it and no longer creates the tooltip just to hide it. This change depends on `@editorjs/ui-kit` `^2.0.1`. ## Migration Plan diff --git a/openspec/changes/editor-destroy/proposal.md b/openspec/changes/archive/2026-10-16-editor-destroy/proposal.md similarity index 94% rename from openspec/changes/editor-destroy/proposal.md rename to openspec/changes/archive/2026-10-16-editor-destroy/proposal.md index 38bb4f60..fb8012af 100644 --- a/openspec/changes/editor-destroy/proposal.md +++ b/openspec/changes/archive/2026-10-16-editor-destroy/proposal.md @@ -10,7 +10,7 @@ There is no way to tear an editor down. Neither `Core` nor `EditorJS` has a `des - `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. +- 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 (merged), 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. @@ -28,10 +28,11 @@ None. - `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()`. +- `ui`: the UI plugin listener lifetime requirement (from #174) gains a scenario for releasing inline tool instances with their popover. ## 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. +- **Related work**: #174 (UI plugin teardown, merged), card "Clean up" on the DocumentModel board. diff --git a/openspec/changes/editor-destroy/specs/architecture/spec.md b/openspec/changes/archive/2026-10-16-editor-destroy/specs/architecture/spec.md similarity index 54% rename from openspec/changes/editor-destroy/specs/architecture/spec.md rename to openspec/changes/archive/2026-10-16-editor-destroy/specs/architecture/spec.md index e8633ab1..25e020be 100644 --- a/openspec/changes/editor-destroy/specs/architecture/spec.md +++ b/openspec/changes/archive/2026-10-16-editor-destroy/specs/architecture/spec.md @@ -1,7 +1,7 @@ ## 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. +Every first-party component that has a teardown (`Core` itself, its services, adapters, plugins, UI plugins, the collaboration client, 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. Listeners registered on objects that the component owns or that the editor hands to it, and that go away with those objects, are outside this rule: the model's listeners between its own nodes, or a tool's listener on its block adapter, are released when the model or the adapter is. 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. diff --git a/openspec/changes/editor-destroy/specs/core/spec.md b/openspec/changes/archive/2026-10-16-editor-destroy/specs/core/spec.md similarity index 100% rename from openspec/changes/editor-destroy/specs/core/spec.md rename to openspec/changes/archive/2026-10-16-editor-destroy/specs/core/spec.md diff --git a/openspec/changes/editor-destroy/specs/dom-adapters/spec.md b/openspec/changes/archive/2026-10-16-editor-destroy/specs/dom-adapters/spec.md similarity index 100% rename from openspec/changes/editor-destroy/specs/dom-adapters/spec.md rename to openspec/changes/archive/2026-10-16-editor-destroy/specs/dom-adapters/spec.md diff --git a/openspec/changes/editor-destroy/specs/editorjs-bundle/spec.md b/openspec/changes/archive/2026-10-16-editor-destroy/specs/editorjs-bundle/spec.md similarity index 100% rename from openspec/changes/editor-destroy/specs/editorjs-bundle/spec.md rename to openspec/changes/archive/2026-10-16-editor-destroy/specs/editorjs-bundle/spec.md diff --git a/openspec/changes/editor-destroy/specs/sdk/spec.md b/openspec/changes/archive/2026-10-16-editor-destroy/specs/sdk/spec.md similarity index 100% rename from openspec/changes/editor-destroy/specs/sdk/spec.md rename to openspec/changes/archive/2026-10-16-editor-destroy/specs/sdk/spec.md diff --git a/openspec/changes/archive/2026-10-16-editor-destroy/specs/ui/spec.md b/openspec/changes/archive/2026-10-16-editor-destroy/specs/ui/spec.md new file mode 100644 index 00000000..f699f05f --- /dev/null +++ b/openspec/changes/archive/2026-10-16-editor-destroy/specs/ui/spec.md @@ -0,0 +1,32 @@ +## MODIFIED Requirements + +### Requirement: UI plugin listener lifetime +Each UI plugin SHALL unsubscribe its own EventBus and DOM listeners when destroyed, detach its owned holder, and dispose its owned popover. Destroying one instance SHALL leave other consumers on the shared EventBus active. Teardown SHALL be safe to repeat. + +#### Scenario: Events after teardown +- **GIVEN** a UI plugin has been destroyed +- **WHEN** the shared EventBus emits a previously subscribed event or a retained DOM element receives an event +- **THEN** that instance no longer renders, moves elements, or dispatches UI actions +- **AND** unrelated listeners and a replacement plugin still receive their events + +#### Scenario: Pending inline toolbar configuration +- **GIVEN** inline tool configuration is still resolving when the toolbar is destroyed +- **WHEN** the configuration finishes resolving +- **THEN** no popover is created, shown, or positioned by the destroyed toolbar + +#### Scenario: Opening a destroyed toolbox +- **GIVEN** `ToolboxUI` has been destroyed +- **WHEN** its `open()` is called +- **THEN** no popover is shown and no `ToolboxOpenedUIEvent` is dispatched + +#### Scenario: Repeated teardown +- **GIVEN** a UI plugin has been destroyed +- **WHEN** `destroy()` is called again +- **THEN** it returns without releasing anything a second time + +#### Scenario: Inline tool instances are not destroyed with their popover +- **GIVEN** the inline toolbar builds its popover from inline tool instances, which their facades share with every other consumer +- **WHEN** the popover is rebuilt for a new selection, the toolbar is hidden, a render is superseded before it finishes, or the toolbar is destroyed +- **THEN** no inline tool instance's `destroy()` is called by the toolbar; each tool's facade destroys its instance once, on editor teardown + +Validated by `src/lifecycle.spec.ts` using real DOM/EventBus listeners and a substituted popover boundary. Vendor-owned global listener internals are outside this requirement's verification scope. diff --git a/openspec/changes/editor-destroy/tasks.md b/openspec/changes/archive/2026-10-16-editor-destroy/tasks.md similarity index 81% rename from openspec/changes/editor-destroy/tasks.md rename to openspec/changes/archive/2026-10-16-editor-destroy/tasks.md index 545b1dce..b2861c14 100644 --- a/openspec/changes/editor-destroy/tasks.md +++ b/openspec/changes/archive/2026-10-16-editor-destroy/tasks.md @@ -1,30 +1,30 @@ ## 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 +- [x] 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.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 +- [x] 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 +- [x] 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` +- [x] 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()` +- [x] 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` +- [x] 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` +- [x] 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` +- [x] 4.4 Implement `FormattingAdapter.destroy()`: abort its controller, then drop the inline tools without destroying them (design D7) +- [x] 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` +- [x] 4.6 Implement `DOMAdapters.destroy()` ## 5. Core services teardown @@ -43,18 +43,18 @@ ## 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 +- [x] 7.1 UI plugin teardown is covered by #174 (merged): no UI plugin teardown in this change +- [x] 7.2 Test that `InlineToolbarUI` never destroys the shared inline tool instance, through a real `InlineToolFacade`, across rebuild, hide, a superseded render and destroy, in `lifecycle.spec.ts` ## 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) +- [x] 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 +- [x] 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 +- [x] 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 +- [x] 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` +- [x] 9.2 Run `yarn lint` and the test suites for every touched package +- [x] 9.3 Run `openspec validate editor-destroy --type change` and make sure it passes diff --git a/openspec/specs/architecture/spec.md b/openspec/specs/architecture/spec.md index e691e94b..d11e0bcf 100644 --- a/openspec/specs/architecture/spec.md +++ b/openspec/specs/architecture/spec.md @@ -3,9 +3,7 @@ ## Purpose Cross-cutting structural invariants that span multiple packages and aren't owned by any single capability spec: the dependency direction between packages, the event-channel naming convention used across the `EventBus`, the single-adapter contract `core` enforces, and which undo/redo implementation takes precedence when collaboration is enabled. These constrain how packages may depend on and communicate with each other; package-specific behavior lives in the respective capability specs ([[model-types]], [[sdk]], [[model]], [[dom-adapters]], [[collaboration-manager]], [[core]], [[ui]], [[ot-server]], [[tools]], [[editorjs-bundle]]). - ## Requirements - ### Requirement: Tools and plugins depend on the SDK, not on model or core Tool and plugin packages SHALL depend on `@editorjs/sdk` (and, where DOM binding is needed, `@editorjs/dom-adapters`) for their contracts, and SHALL NOT declare a direct dependency on `@editorjs/model` or `@editorjs/core`. @@ -75,3 +73,24 @@ When `@editorjs/collaboration-manager` is registered, it SHALL intercept `core:u - **THEN** no listener calls `preventDefault()`, so `core`'s own `UndoRedoManager` performs the undo/redo Implemented in `packages/collaboration-manager/src/CollaborationManager.ts` (constructor's `onUndo`/`onRedo` handlers), `packages/core/src/components/UndoRedoManager.ts`. + +### Requirement: Listener teardown through AbortController +Every first-party component that has a teardown (`Core` itself, its services, adapters, plugins, UI plugins, the collaboration client, 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. Listeners registered on objects that the component owns or that the editor hands to it, and that go away with those objects, are outside this rule: the model's listeners between its own nodes, or a tool's listener on its block adapter, are released when the model or the adapter is. + +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/specs/core/spec.md b/openspec/specs/core/spec.md index e30db5f9..72868f05 100644 --- a/openspec/specs/core/spec.md +++ b/openspec/specs/core/spec.md @@ -60,7 +60,7 @@ The system SHALL expose an `EditorAPI` aggregating `BlocksAPI` (insert/insertMan Implemented in `src/api/index.ts`, `src/api/BlocksAPI.ts`, `src/api/SelectionAPI.ts`, `src/api/DocumentAPI/DocumentAPI.ts`, `src/api/TextAPI.ts`, `src/components/BlockManager.ts`, validated by co-located `.spec.ts`/`.integration.spec.ts` files. ### 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`. +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 @@ -72,6 +72,21 @@ The system SHALL provide `BlockRenderer`, which listens for model `BlockAddedEve - **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`. ### Requirement: Selection and inline formatting @@ -136,3 +151,56 @@ Implemented in `src/components/UndoRedoManager.ts`, validated by its co-located - **WHEN** each plugin's `destroy()` runs - **THEN** its registry entry is removed so no stale public API remains reachable +### 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 + diff --git a/openspec/specs/dom-adapters/spec.md b/openspec/specs/dom-adapters/spec.md index 866ab459..833797ee 100644 --- a/openspec/specs/dom-adapters/spec.md +++ b/openspec/specs/dom-adapters/spec.md @@ -3,9 +3,7 @@ ## Purpose `@editorjs/dom-adapters` is the glue layer between the document model and real browser DOM elements. It has no rendering/UI logic of its own: it listens to model change events (text added/removed, formatted/unformatted, caret updates) and mutates the DOM to match, and listens to native DOM events (`beforeinput`, `selectionchange`) and translates them into model API calls. It is exposed as an `EditorJSAdapterPlugin` (`DOMAdapters`) wired through an Inversify DI container. - ## Requirements - ### Requirement: DOMAdapters plugin entry point The system SHALL provide `DOMAdapters` as the `EditorJSAdapterPlugin` entry point that builds the DI container, creates/destroys a `DOMBlockToolAdapter` per block, and instantiates the singleton `FormattingAdapter` and `CaretAdapter`. @@ -80,3 +78,27 @@ The system SHALL provide `FormattingAdapter`, which registers `InlineTool` insta - **THEN** it expands the range by one character on each side to catch adjacent boundary fragments, extracts the range into a template, wraps overlapping fragments using each tool's `createWrapper(data)` output, and replaces the DOM range contents so overlapping/adjacent inline formats nest correctly Implemented in `src/FormattingAdapter/index.ts`, `src/utils/surround.ts`, `src/utils/expandRangeNodeBoundary.ts`. + +### 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/specs/editorjs-bundle/spec.md b/openspec/specs/editorjs-bundle/spec.md index 1656c411..27fc5612 100644 --- a/openspec/specs/editorjs-bundle/spec.md +++ b/openspec/specs/editorjs-bundle/spec.md @@ -47,3 +47,31 @@ The `EditorJS` class SHALL accept a v2-style `config.tools` map of tool name to - **WHEN** the editor is composed - **THEN** it throws an error rather than silently registering the tool under a name that won't match `defaultBlock` or other lookups +### 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/specs/sdk/spec.md b/openspec/specs/sdk/spec.md index cf0cbc8f..76a53641 100644 --- a/openspec/specs/sdk/spec.md +++ b/openspec/specs/sdk/spec.md @@ -62,23 +62,33 @@ The system SHALL provide a `ToolsCollection` (a `Map` s Implemented in `src/tools/ToolsCollection.ts`. ### 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()`. +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** it unsubscribes its model listener so no further translated events are dispatched +- **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`). +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` @@ -134,3 +144,21 @@ The system SHALL declare two empty, augmentable interfaces — `EditorjsPluginAp Implemented in `src/entities/BaseTool.ts`, `src/tools/facades/BaseToolFacade.ts`. +### 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 + diff --git a/openspec/specs/ui/spec.md b/openspec/specs/ui/spec.md index 84e2baed..e3a39fb3 100644 --- a/openspec/specs/ui/spec.md +++ b/openspec/specs/ui/spec.md @@ -111,7 +111,6 @@ The system SHALL provide `ToolboxUI`, a searchable popover of block tools popula Implemented in `src/Toolbox/Toolbox.ts`, `src/Toolbox/events/*`, `src/Toolbox/ToolboxConfigEntry.ts`. - ### Requirement: UI plugin listener lifetime Each UI plugin SHALL unsubscribe its own EventBus and DOM listeners when destroyed, detach its owned holder, and dispose its owned popover. Destroying one instance SHALL leave other consumers on the shared EventBus active. Teardown SHALL be safe to repeat. @@ -136,6 +135,11 @@ Each UI plugin SHALL unsubscribe its own EventBus and DOM listeners when destroy - **WHEN** `destroy()` is called again - **THEN** it returns without releasing anything a second time +#### Scenario: Inline tool instances are not destroyed with their popover +- **GIVEN** the inline toolbar builds its popover from inline tool instances, which their facades share with every other consumer +- **WHEN** the popover is rebuilt for a new selection, the toolbar is hidden, a render is superseded before it finishes, or the toolbar is destroyed +- **THEN** no inline tool instance's `destroy()` is called by the toolbar; each tool's facade destroys its instance once, on editor teardown + Validated by `src/lifecycle.spec.ts` using real DOM/EventBus listeners and a substituted popover boundary. Vendor-owned global listener internals are outside this requirement's verification scope. ### Requirement: Accessible blocks holder diff --git a/packages/collaboration-manager/src/CollaborationManager.spec.ts b/packages/collaboration-manager/src/CollaborationManager.spec.ts index a1c2d2bc..7ee6c108 100644 --- a/packages/collaboration-manager/src/CollaborationManager.spec.ts +++ b/packages/collaboration-manager/src/CollaborationManager.spec.ts @@ -1364,18 +1364,50 @@ describe('CollaborationManager', () => { }); describe('destroy', () => { - it('should unsubscribe model and core event listeners', () => { + it('should unsubscribe from model updates', () => { const model = new EditorJSModel(userId, { identifier: documentId }); const removeModelListenerSpy = jest.spyOn(model, 'removeEventListener'); - const { manager, eventBus } = createManager(config as Required, model); - const removeEventBusListenerSpy = jest.spyOn(eventBus, 'removeEventListener'); + const { manager } = createManager(config as Required, model); manager.destroy(); expect(removeModelListenerSpy).toHaveBeenCalledWith(EventType.Changed, expect.any(Function)); - expect(removeEventBusListenerSpy).toHaveBeenCalledWith(`core:${CoreEventType.Undo}`, expect.any(Function)); - expect(removeEventBusListenerSpy).toHaveBeenCalledWith(`core:${CoreEventType.Redo}`, expect.any(Function)); - expect(removeEventBusListenerSpy).toHaveBeenCalledWith(`core:${CoreEventType.Ready}`, expect.any(Function)); + }); + + it('should stop handling core undo and redo events', () => { + const model = new EditorJSModel(userId, { identifier: documentId }); + const { manager, eventBus } = createManager(config as Required, model); + const undo = jest.spyOn(manager, 'undo'); + const redo = jest.spyOn(manager, 'redo'); + + manager.destroy(); + + const undoEvent = new CustomEvent(`core:${CoreEventType.Undo}`, { cancelable: true }); + const redoEvent = new CustomEvent(`core:${CoreEventType.Redo}`, { cancelable: true }); + + eventBus.dispatchEvent(undoEvent); + eventBus.dispatchEvent(redoEvent); + + expect(undo).not.toHaveBeenCalled(); + expect(redo).not.toHaveBeenCalled(); + expect(undoEvent.defaultPrevented).toBe(false); + expect(redoEvent.defaultPrevented).toBe(false); + }); + + it('should not connect when the editor becomes ready after destroy', async () => { + const model = new EditorJSModel(userId, { identifier: documentId }); + const { manager, eventBus } = createManager({ + ...config, + collaborationServer: 'ws://test-collab.invalid/document', + } as Required, model); + const connectSpy = jest.spyOn(OTClient.prototype, 'connectDocument'); + + manager.destroy(); + eventBus.dispatchEvent(new CustomEvent(`core:${CoreEventType.Ready}`)); + await Promise.resolve(); + + expect(connectSpy).not.toHaveBeenCalled(); + connectSpy.mockRestore(); }); it('should close ot client connection', async () => { diff --git a/packages/collaboration-manager/src/CollaborationManager.ts b/packages/collaboration-manager/src/CollaborationManager.ts index 75d8b82d..8cf8ce98 100644 --- a/packages/collaboration-manager/src/CollaborationManager.ts +++ b/packages/collaboration-manager/src/CollaborationManager.ts @@ -71,24 +71,9 @@ export class CollaborationManager implements EditorjsPlugin { #debounceTimer?: ReturnType; /** - * Cleanup callback for document updates listener + * Releases the document, undo, redo and ready listeners on destroy */ - #unsubscribeDocumentUpdates?: () => void; - - /** - * Cleanup callback for undo event listener - */ - #unsubscribeUndo?: () => void; - - /** - * Cleanup callback for redo event listener - */ - #unsubscribeRedo?: () => void; - - /** - * Cleanup callback for ready event listener - */ - #unsubscribeReady?: () => void; + readonly #controller = new AbortController(); /** * Editor's config @@ -106,30 +91,23 @@ export class CollaborationManager implements EditorjsPlugin { this.#config = config; this.#undoRedoManager = new UndoRedoManager(); - const onUndo = (e: UndoCoreEvent): void => { + const { signal } = this.#controller; + + signal.addEventListener('abort', api.document.onUpdate(this.#handleEvent.bind(this))); + + eventBus.addEventListener(`core:${CoreEventType.Undo}`, (e: UndoCoreEvent) => { e.preventDefault(); this.undo(); - }; - const onRedo = (e: RedoCoreEvent): void => { + }, { signal }); + + eventBus.addEventListener(`core:${CoreEventType.Redo}`, (e: RedoCoreEvent) => { e.preventDefault(); this.redo(); - }; - const onReady = (): void => { - this.#connect(); - }; - - this.#unsubscribeDocumentUpdates = api.document.onUpdate(this.#handleEvent.bind(this)); - - eventBus.addEventListener(`core:${CoreEventType.Undo}`, onUndo); - this.#unsubscribeUndo = () => void eventBus.removeEventListener(`core:${CoreEventType.Undo}`, onUndo); - - eventBus.addEventListener(`core:${CoreEventType.Redo}`, onRedo); - this.#unsubscribeRedo = () => void eventBus.removeEventListener(`core:${CoreEventType.Redo}`, onRedo); + }, { signal }); - eventBus.addEventListener(`core:${CoreEventType.Ready}`, onReady); - this.#unsubscribeReady = () => void eventBus.removeEventListener(`core:${CoreEventType.Ready}`, onReady); + eventBus.addEventListener(`core:${CoreEventType.Ready}`, () => this.#connect(), { signal }); } /** @@ -370,14 +348,11 @@ export class CollaborationManager implements EditorjsPlugin { } /** - * Destroys the plugin instance: clears the debounce timer + * Destroys the plugin instance: clears the debounce timer, removes its listeners and closes the connection */ public destroy(): void { clearTimeout(this.#debounceTimer); - this.#unsubscribeDocumentUpdates?.(); - this.#unsubscribeUndo?.(); - this.#unsubscribeRedo?.(); - this.#unsubscribeReady?.(); + this.#controller.abort(); this.#client?.close(); this.#client = null; } diff --git a/packages/collaboration-manager/src/client/OTClient.spec.ts b/packages/collaboration-manager/src/client/OTClient.spec.ts index 018ff399..523865b9 100644 --- a/packages/collaboration-manager/src/client/OTClient.spec.ts +++ b/packages/collaboration-manager/src/client/OTClient.spec.ts @@ -230,5 +230,72 @@ describe('OTClient', () => { expect(closeSpy).toHaveBeenCalledTimes(1); closeSpy.mockRestore(); }); + + it('should close a socket that is still connecting', async () => { + const client = new OTClient('ws://test-collab.invalid/document', userId, jest.fn(), jest.fn()); + const closeSpy = jest.spyOn(MockWebSocket.lastInstance!, 'close'); + + client.close(); + await Promise.resolve(); + await Promise.resolve(); + + expect(closeSpy).toHaveBeenCalledTimes(1); + closeSpy.mockRestore(); + }); + + it('should release its socket listeners on close', async () => { + const client = new OTClient('ws://test-collab.invalid/document', userId, jest.fn(), jest.fn()); + + await client.connectDocument(stubDocument); + + const socket = MockWebSocket.lastInstance!; + + client.close(); + await Promise.resolve(); + + expect(socket.listenerCount('open')).toBe(0); + expect(socket.listenerCount('message')).toBe(0); + }); + + it('should release a pending handshake listener on close', async () => { + const client = new OTClient('ws://test-collab.invalid/document', userId, jest.fn(), jest.fn()); + + await Promise.resolve(); + + const socket = MockWebSocket.lastInstance!; + const send = jest.spyOn(socket, 'send').mockImplementation(() => undefined); + + await client.connectDocument(stubDocument); + + expect(socket.listenerCount('message')).toBe(2); + + client.close(); + + expect(socket.listenerCount('message')).toBe(0); + send.mockRestore(); + }); + + it('should release a pending acknowledgement listener on close', async () => { + const client = new OTClient('ws://test-collab.invalid/document', userId, jest.fn(), jest.fn()); + + await client.connectDocument(stubDocument); + + const socket = MockWebSocket.lastInstance!; + const send = jest.spyOn(socket, 'send').mockImplementation(() => undefined); + const index = Index.text([{ blockIndex: 0, + dataKey: createDataKey('text'), + textRange: [0, 0], + documentId }]); + + await client.send(new Operation(OperationType.Insert, index, { payload: 'a' }, userId, 0)); + + expect(socket.listenerCount('message')).toBe(2); + + client.close(); + await Promise.resolve(); + + expect(socket.listenerCount('message')).toBe(0); + send.mockRestore(); + }); }); }); diff --git a/packages/collaboration-manager/src/client/OTClient.ts b/packages/collaboration-manager/src/client/OTClient.ts index 708a21be..887e25d0 100644 --- a/packages/collaboration-manager/src/client/OTClient.ts +++ b/packages/collaboration-manager/src/client/OTClient.ts @@ -27,6 +27,16 @@ export class OTClient { */ #resolvedOperations: Operation[] = []; + /** + * Releases every socket listener on close, including one-shot handlers still awaiting a reply + */ + readonly #controller = new AbortController(); + + /** + * The socket itself, so close() can close it even before it opens + */ + #socket: WebSocket; + /** * Promise resolving the WebSocket client */ @@ -66,12 +76,16 @@ export class OTClient { this.#userId = userId; this.#onRemoteOperation = onRemoteOperation; this.#onHandshake = onHandshake; + const ws = new WebSocket(serverAddr); + + this.#socket = ws; + this.#ws = new Promise((resolve) => { - const ws = new WebSocket(serverAddr); + const { signal } = this.#controller; ws.addEventListener('open', () => { resolve(ws); - }); + }, { signal }); ws.addEventListener('message', (message) => { try { @@ -79,7 +93,7 @@ export class OTClient { } catch (e) { console.error('[OTClient] Couldn\'t process a message', message.data); } - }); + }, { signal }); }); } @@ -113,7 +127,7 @@ export class OTClient { } }; - ws.addEventListener('message', onMessage); + ws.addEventListener('message', onMessage, { signal: this.#controller.signal }); }); ws.send(JSON.stringify({ @@ -143,9 +157,8 @@ export class OTClient { * Closes websocket connection */ public close(): void { - void this.#ws.then((ws) => { - ws.close(); - }); + this.#controller.abort(); + this.#socket.close(); } /** @@ -197,7 +210,7 @@ export class OTClient { } }; - ws.addEventListener('message', onMessage); + ws.addEventListener('message', onMessage, { signal: this.#controller.signal }); ws.send(JSON.stringify({ type: MessageType.Operation, diff --git a/packages/collaboration-manager/test/mocks/ws.ts b/packages/collaboration-manager/test/mocks/ws.ts index 8662a352..c8f2232c 100644 --- a/packages/collaboration-manager/test/mocks/ws.ts +++ b/packages/collaboration-manager/test/mocks/ws.ts @@ -34,11 +34,16 @@ export class MockWebSocket { * @param type - event type * @param listener - listener function */ - public addEventListener(type: string, listener: WsListener | EventListenerObject): void { - if (typeof listener !== 'function') { + public addEventListener(type: string, listener: WsListener | EventListenerObject, options?: AddEventListenerOptions): void { + if (typeof listener !== 'function' || options?.signal?.aborted === true) { return; } + /** + * Honours `signal` like a real EventTarget, so aborting it removes the listener + */ + options?.signal?.addEventListener('abort', () => this.removeEventListener(type, listener), { once: true }); + let set = this.listeners.get(type); if (!set) { @@ -54,6 +59,14 @@ export class MockWebSocket { * @param type - event type * @param listener - listener function */ + /** + * Number of listeners currently registered for `type` + * @param type - event type + */ + public listenerCount(type: string): number { + return this.listeners.get(type)?.size ?? 0; + } + public removeEventListener(type: string, listener: WsListener | EventListenerObject): void { if (typeof listener !== 'function') { return; diff --git a/packages/core/src/index.spec.ts b/packages/core/src/index.spec.ts index 4fd4c8f3..d7f3cda1 100644 --- a/packages/core/src/index.spec.ts +++ b/packages/core/src/index.spec.ts @@ -404,21 +404,6 @@ describe('Core', () => { 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(); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 22b344ca..6ecf394c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -293,7 +293,7 @@ export default class Core { this.#safely('UndoRedoManager', () => undoRedoManager?.destroy()); this.#safely('SelectionManager', () => selectionManager?.destroy()); this.#safely('BlockRenderer', () => blockRenderer?.destroy()); - this.#safely('adapter', () => adapter?.destroy?.()); + this.#safely('adapter', () => adapter?.destroy()); /** * Last: the shared inline tool instances are used by the adapter and the UI until they're gone diff --git a/packages/dom-adapters/package.json b/packages/dom-adapters/package.json index da5c314b..5b065577 100644 --- a/packages/dom-adapters/package.json +++ b/packages/dom-adapters/package.json @@ -12,9 +12,9 @@ "lint": "eslint ./src", "lint:ci": "yarn lint --max-warnings 0", "lint:fix": "yarn lint --fix", - "test": "jest", + "test": "node --experimental-vm-modules $(yarn bin jest)", "test:coverage": "yarn test --coverage=true", - "test:mutations": "stryker run", + "test:mutations": "NODE_OPTIONS=\"$NODE_OPTIONS --experimental-vm-modules\" stryker run", "clear": "rm -rf ./dist && rm -rf ./tsconfig.build.tsbuildinfo" }, "devDependencies": { diff --git a/packages/dom-adapters/src/BlockToolAdapter/index.ts b/packages/dom-adapters/src/BlockToolAdapter/index.ts index a232b887..9b0a24b0 100644 --- a/packages/dom-adapters/src/BlockToolAdapter/index.ts +++ b/packages/dom-adapters/src/BlockToolAdapter/index.ts @@ -46,11 +46,6 @@ export class DOMBlockToolAdapter extends BlockToolAdapter { #formattingAdapter: FormattingAdapter; #inputsRegistry: InputsRegistry; - /** - * Stored reference to the beforeinput event listener so it can be removed on destroy. - */ - #beforeInputListener: EventListener; - /** * Editor API instance */ @@ -81,25 +76,14 @@ export class DOMBlockToolAdapter extends BlockToolAdapter { this.#api = api; /** - * @param event - BeforeInputEvent - */ - this.#beforeInputListener = ((event: BeforeInputUIEvent) => { - this.#processDelegatedBeforeInput(event); - }) as EventListener; - - /** + * @param event - beforeinput event delegated by the UI * @todo Needs to be documented. If UI module is replaced and doesn't dispatch the event nothing would work */ - eventBus.addEventListener(`ui:${BeforeInputUIEventName}`, this.#beforeInputListener); - } - - /** - * Releases all resources held by this adapter. - * Removes both the model change listener (via super) and the eventBus beforeinput listener. - */ - public override destroy(): void { - super.destroy(); - this.eventBus.removeEventListener(`ui:${BeforeInputUIEventName}`, this.#beforeInputListener); + eventBus.addEventListener( + `ui:${BeforeInputUIEventName}`, + ((event: BeforeInputUIEvent) => this.#processDelegatedBeforeInput(event)) as EventListener, + { signal: this.signal } + ); } /** diff --git a/packages/dom-adapters/src/CaretAdapter/CaretAdapter.spec.ts b/packages/dom-adapters/src/CaretAdapter/CaretAdapter.spec.ts new file mode 100644 index 00000000..c3b9a1cd --- /dev/null +++ b/packages/dom-adapters/src/CaretAdapter/CaretAdapter.spec.ts @@ -0,0 +1,124 @@ +import 'reflect-metadata'; +import { afterEach, describe, expect, it, jest } from '@jest/globals'; +import type { CoreConfig, DataKey, EditorAPI } from '@editorjs/sdk'; +import { Index } from '@editorjs/sdk'; + +/** + * The DOM helpers package ships extensionless ESM imports that Node can't resolve + */ +jest.unstable_mockModule('@editorjs/dom', () => ({ + isNativeInput: jest.fn(() => false), +})); + +const { CaretAdapter } = await import('./index.js'); +const { InputsRegistry } = await import('../InputsRegistry/index.js'); + +/** + * Stand-in for `document`, where the shared selection watcher adds its listener + */ +const fakeDocument = { + addEventListener: jest.fn(), + removeEventListener: jest.fn(), +}; + +Object.assign(globalThis, { document: fakeDocument }); + +/** + * Adapter under test with handles on its API stubs + */ +interface Harness { + adapter: InstanceType; + unsubscribeCaret: jest.Mock; + getIdByIndex: jest.Mock; + emitCaretUpdate: (event: unknown) => void; +} + +/** + * Adapters created by a test, destroyed after it so the shared selection watcher starts empty + */ +const created: InstanceType[] = []; + +/** + * Creates an adapter with a stub API recording its caret subscription + */ +function createAdapter(): Harness { + const unsubscribeCaret = jest.fn(); + const getIdByIndex = jest.fn(() => undefined); + let onCaretUpdate: (event: unknown) => void = () => undefined; + const api = { + selection: { + createCaret: jest.fn(() => ({ index: null, + userId: 'user' })), + onCaretUpdate: jest.fn((callback: (event: unknown) => void) => { + onCaretUpdate = callback; + + return unsubscribeCaret; + }), + }, + blocks: { getIdByIndex }, + } as unknown as EditorAPI; + + const adapter = new CaretAdapter( + { holder: {} as HTMLElement, + userId: 'user' } as Required, + api, + new InputsRegistry() + ); + + created.push(adapter); + + return { adapter, + unsubscribeCaret, + getIdByIndex, + emitCaretUpdate: event => onCaretUpdate(event) }; +} + +describe('CaretAdapter', () => { + afterEach(() => { + created.splice(0).forEach(adapter => adapter.destroy()); + }); + + it('should look up the input for the current user\'s model caret update', () => { + const { emitCaretUpdate, getIdByIndex } = createAdapter(); + + emitCaretUpdate({ + detail: { + userId: 'user', + index: Index.text([{ blockIndex: 0, + dataKey: 'text' as DataKey, + textRange: [0, 1] }]).serialize(), + }, + }); + + expect(getIdByIndex).toHaveBeenCalledWith(0); + }); + + it('should stop updating the model caret on selectionchange after destroy', () => { + fakeDocument.addEventListener.mockClear(); + + const { adapter } = createAdapter(); + + adapter.destroy(); + + const [call] = fakeDocument.addEventListener.mock.calls.filter(([type]) => type === 'selectionchange'); + + expect((call[2] as AddEventListenerOptions | undefined)?.signal?.aborted).toBe(true); + }); + + it('should not touch the DOM selection on model caret updates after destroy', () => { + const { adapter, unsubscribeCaret } = createAdapter(); + + adapter.destroy(); + + expect(unsubscribeCaret).toHaveBeenCalledTimes(1); + }); + + it('should be safe to destroy twice', () => { + const { adapter, unsubscribeCaret } = createAdapter(); + + adapter.destroy(); + adapter.destroy(); + + expect(unsubscribeCaret).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/dom-adapters/src/CaretAdapter/index.ts b/packages/dom-adapters/src/CaretAdapter/index.ts index a545c8a6..46725cb3 100644 --- a/packages/dom-adapters/src/CaretAdapter/index.ts +++ b/packages/dom-adapters/src/CaretAdapter/index.ts @@ -51,6 +51,11 @@ export class CaretAdapter { */ #config: Required; + /** + * Releases the selection watcher subscription and the caret updates subscription on destroy + */ + readonly #controller = new AbortController(); + /** * @param config - Editor's config * @param api - Editor's API @@ -67,14 +72,20 @@ export class CaretAdapter { this.#container = config.holder; this.#currentUserCaret = this.#api.selection.createCaret(this.#config.userId); - const { on } = useSelectionChange(); + const { signal } = this.#controller; + const { on, off } = useSelectionChange(); - /** - * @todo Unsubscribe on adapter destruction - */ on(this.#container, selection => this.#onSelectionChange(selection), this); + signal.addEventListener('abort', () => off(this.#container)); - this.#api.selection.onCaretUpdate(event => this.#onModelCaretUpdate(event)); + signal.addEventListener('abort', this.#api.selection.onCaretUpdate(event => this.#onModelCaretUpdate(event))); + } + + /** + * Stops watching DOM selection changes and model caret updates + */ + public destroy(): void { + this.#controller.abort(); } /** diff --git a/packages/dom-adapters/src/FormattingAdapter/FormattingAdapter.spec.ts b/packages/dom-adapters/src/FormattingAdapter/FormattingAdapter.spec.ts new file mode 100644 index 00000000..9f3f32c8 --- /dev/null +++ b/packages/dom-adapters/src/FormattingAdapter/FormattingAdapter.spec.ts @@ -0,0 +1,127 @@ +import 'reflect-metadata'; +import { describe, expect, it, jest } from '@jest/globals'; +import type { DataKey, EditorAPI, EventBus, InlineTool, InlineToolName } from '@editorjs/sdk'; +import { CoreEventType, Index, TextFormattedEvent } from '@editorjs/sdk'; +import type { CaretAdapter } from '../CaretAdapter/index.js'; + +/** + * The DOM helpers package ships extensionless ESM imports that Node can't resolve + */ +jest.unstable_mockModule('@editorjs/dom', () => ({ + isNativeInput: jest.fn(() => false), +})); + +const { FormattingAdapter } = await import('./index.js'); + +/** + * Adapter under test with handles on its stubs + */ +interface Harness { + adapter: InstanceType; + eventBus: EventTarget; + unsubscribeUpdates: jest.Mock; + findInput: jest.Mock; + emitModelUpdate: (event: unknown) => void; +} + +/** + * Creates an adapter wired to a real EventTarget standing in for the EventBus + */ +function createAdapter(): Harness { + const unsubscribeUpdates = jest.fn(); + const findInput = jest.fn(() => undefined); + const eventBus = new EventTarget(); + let onUpdate: (event: unknown) => void = () => undefined; + const api = { + document: { + onUpdate: jest.fn((callback: (event: unknown) => void) => { + onUpdate = callback; + + return unsubscribeUpdates; + }), + }, + } as unknown as EditorAPI; + + const adapter = new FormattingAdapter(api, { findInput } as unknown as CaretAdapter, eventBus as EventBus); + + return { adapter, + eventBus, + unsubscribeUpdates, + findInput, + emitModelUpdate: event => onUpdate(event) }; +} + +/** + * Dispatches a ToolLoaded event for an inline tool whose instance is created by `create` + * @param eventBus - bus to dispatch on + * @param create - creates the inline tool instance + */ +function loadInlineTool(eventBus: EventTarget, create: () => object): void { + eventBus.dispatchEvent(Object.assign(new Event(`core:${CoreEventType.ToolLoaded}`), { + detail: { + tool: { + name: 'bold', + isInline: () => true, + create, + }, + }, + })); +} + +describe('FormattingAdapter', () => { + it('should look up the input of a formatted text range on model update', () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => undefined); + const { eventBus, findInput, emitModelUpdate } = createAdapter(); + + loadInlineTool(eventBus, () => ({})); + emitModelUpdate(new TextFormattedEvent( + Index.text([{ blockIndex: 0, + dataKey: 'text' as DataKey, + textRange: [0, 1] }]), + { tool: 'bold' as InlineToolName }, + 'user' + )); + + expect(findInput).toHaveBeenCalledWith(0, 'text'); + + warn.mockRestore(); + }); + + it('should not destroy the shared inline tool instances it uses', () => { + const destroy = jest.fn(); + const { adapter, eventBus } = createAdapter(); + + loadInlineTool(eventBus, () => ({ destroy })); + adapter.destroy(); + + expect(destroy).not.toHaveBeenCalled(); + }); + + it('should ignore core:ToolLoaded after destroy', () => { + const create = jest.fn(() => ({})); + const { adapter, eventBus } = createAdapter(); + + adapter.destroy(); + loadInlineTool(eventBus, create); + + expect(create).not.toHaveBeenCalled(); + }); + + it('should ignore model updates after destroy', () => { + const { adapter, unsubscribeUpdates } = createAdapter(); + + adapter.destroy(); + + expect(unsubscribeUpdates).toHaveBeenCalledTimes(1); + }); + + it('should not destroy a tool instance it replaces', () => { + const destroy = jest.fn(); + const { eventBus } = createAdapter(); + + loadInlineTool(eventBus, () => ({ destroy })); + loadInlineTool(eventBus, () => ({})); + + expect(destroy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/dom-adapters/src/FormattingAdapter/index.ts b/packages/dom-adapters/src/FormattingAdapter/index.ts index 320eb97f..57078c53 100644 --- a/packages/dom-adapters/src/FormattingAdapter/index.ts +++ b/packages/dom-adapters/src/FormattingAdapter/index.ts @@ -40,6 +40,11 @@ export class FormattingAdapter { */ #caretAdapter: CaretAdapter; + /** + * Releases the ToolLoaded and model update listeners on destroy + */ + readonly #controller = new AbortController(); + /** * @param api - Editor's API * @param caretAdapter - caret adapter instance @@ -67,12 +72,15 @@ export class FormattingAdapter { const name = createInlineToolName(tool.name); this.attachTool(name, toolInstance); - }); + }, { signal: this.#controller.signal }); /** * Add event listener for model changes */ - this.#api.document.onUpdate((event: ModelEvents) => this.#handleModelUpdates(event)); + this.#controller.signal.addEventListener( + 'abort', + this.#api.document.onUpdate((event: ModelEvents) => this.#handleModelUpdates(event)) + ); } /** @@ -113,6 +121,18 @@ export class FormattingAdapter { this.#tools.set(toolName, tool); } + /** + * Stops listening to tool and model events and drops the attached inline tool instances + */ + public destroy(): void { + this.#controller.abort(); + + /** + * The attached instances are the tool facades' shared ones: their facades destroy them on editor teardown + */ + this.#tools.clear(); + } + /** * Detaches InlineTool from the adapter * @param toolName - name of the tool to be detached diff --git a/packages/dom-adapters/src/index.spec.ts b/packages/dom-adapters/src/index.spec.ts index 12fd3a8c..d21564b7 100644 --- a/packages/dom-adapters/src/index.spec.ts +++ b/packages/dom-adapters/src/index.spec.ts @@ -1,5 +1,125 @@ -describe('Example test', () => { - it('should pass', () => { - expect(true).toBe(true); +import 'reflect-metadata'; +import { describe, expect, it, jest } from '@jest/globals'; +import type { BlockId, EditorAPI, EditorjsPluginParams, EventBus } from '@editorjs/sdk'; + +jest.unstable_mockModule('@editorjs/dom', () => ({ + isNativeInput: jest.fn(() => false), +})); + +const { DOMAdapters } = await import('./index.js'); +const { DOMBlockToolAdapter } = await import('./BlockToolAdapter/index.js'); +const { CaretAdapter } = await import('./CaretAdapter/index.js'); +const { FormattingAdapter } = await import('./FormattingAdapter/index.js'); +const { InputsRegistry } = await import('./InputsRegistry/index.js'); + +/** + * Stand-in for `document`, where the shared selection watcher adds its listener + */ +Object.assign(globalThis, { + document: { + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + }, +}); + +/** + * Creates the plugin with stub API members the adapters subscribe through + */ +function createPlugin(): { plugin: InstanceType; + eventBus: EventTarget; } { + const eventBus = new EventTarget(); + const api = { + selection: { + createCaret: jest.fn(() => ({ index: null })), + onCaretUpdate: jest.fn(() => jest.fn()), + }, + document: { + onUpdate: jest.fn(() => jest.fn()), + }, + } as unknown as EditorAPI; + + const plugin = new DOMAdapters({ + config: { holder: {} as HTMLElement, + userId: 'user' }, + api, + eventBus: eventBus as EventBus, + } as unknown as EditorjsPluginParams); + + return { plugin, + eventBus }; +} + +describe('DOMAdapters', () => { + it('should destroy every remaining block adapter on destroy', () => { + const destroy = jest.spyOn(DOMBlockToolAdapter.prototype, 'destroy'); + const { plugin } = createPlugin(); + + plugin.createBlockToolAdapter('a' as BlockId, 'paragraph'); + plugin.createBlockToolAdapter('b' as BlockId, 'paragraph'); + destroy.mockClear(); + + plugin.destroy(); + + expect(destroy).toHaveBeenCalledTimes(2); + + destroy.mockRestore(); + }); + + it('should leave the inputs registry empty after destroy', () => { + const removeBlock = jest.spyOn(InputsRegistry.prototype, 'removeBlock'); + const { plugin } = createPlugin(); + + plugin.createBlockToolAdapter('a' as BlockId, 'paragraph'); + plugin.createBlockToolAdapter('b' as BlockId, 'paragraph'); + + plugin.destroy(); + + expect(removeBlock).toHaveBeenCalledWith('a'); + expect(removeBlock).toHaveBeenCalledWith('b'); + + removeBlock.mockRestore(); + }); + + it('should not destroy a block adapter already destroyed with its block', () => { + const destroy = jest.spyOn(DOMBlockToolAdapter.prototype, 'destroy'); + const { plugin } = createPlugin(); + + plugin.createBlockToolAdapter('a' as BlockId, 'paragraph'); + plugin.destroyBlockToolAdapter('a' as BlockId); + destroy.mockClear(); + + plugin.destroy(); + + expect(destroy).not.toHaveBeenCalled(); + + destroy.mockRestore(); + }); + + it('should destroy caret and formatting adapters on destroy', () => { + const caretDestroy = jest.spyOn(CaretAdapter.prototype, 'destroy'); + const formattingDestroy = jest.spyOn(FormattingAdapter.prototype, 'destroy'); + const { plugin } = createPlugin(); + + plugin.destroy(); + + expect(caretDestroy).toHaveBeenCalledTimes(1); + expect(formattingDestroy).toHaveBeenCalledTimes(1); + + caretDestroy.mockRestore(); + formattingDestroy.mockRestore(); + }); + + it('should release the ui:before-input listener of block adapters on destroy', () => { + const { plugin, eventBus } = createPlugin(); + const addEventListener = jest.spyOn(eventBus, 'addEventListener'); + + plugin.createBlockToolAdapter('a' as BlockId, 'paragraph'); + + const call = addEventListener.mock.calls.find(([type]) => type === 'ui:before-input') as unknown[]; + const options = call[2] as AddEventListenerOptions; + + plugin.destroy(); + + expect(options.signal?.aborted).toBe(true); }); }); diff --git a/packages/dom-adapters/src/index.ts b/packages/dom-adapters/src/index.ts index 14e5b997..03ee66da 100644 --- a/packages/dom-adapters/src/index.ts +++ b/packages/dom-adapters/src/index.ts @@ -84,6 +84,19 @@ export class DOMAdapters implements EditorJSAdapterPlugin { return adapter; } + /** + * Destroys every block adapter still alive, then stops the caret and formatting adapters. + * Called by Core after every rendered block has been released + */ + public destroy(): void { + for (const blockId of [...this.#adapters.keys()]) { + this.destroyBlockToolAdapter(blockId); + } + + this.#iocContainer.get(CaretAdapter).destroy(); + this.#iocContainer.get(FormattingAdapter).destroy(); + } + /** * Destroys the BlockToolAdapter for the given block. * Cleans up all inputs registered for the block and removes the adapter instance. diff --git a/packages/dom-adapters/src/utils/selectionRangeInInput.spec.ts b/packages/dom-adapters/src/utils/selectionRangeInInput.spec.ts index ef90fb2f..b5a86f67 100644 --- a/packages/dom-adapters/src/utils/selectionRangeInInput.spec.ts +++ b/packages/dom-adapters/src/utils/selectionRangeInInput.spec.ts @@ -1,17 +1,18 @@ /* eslint-disable @typescript-eslint/no-magic-numbers, jsdoc/require-jsdoc */ import { describe, it, expect, jest, beforeEach } from '@jest/globals'; -import { getAbsoluteRangeOffset } from './getAbsoluteRangeOffset.js'; -import { + +jest.unstable_mockModule('./getAbsoluteRangeOffset.js', () => ({ + getAbsoluteRangeOffset: jest.fn(), +})); + +const { getAbsoluteRangeOffset } = await import('./getAbsoluteRangeOffset.js'); +const { getClippedTextRangeForInput, isInputContainsOnlyEndOfSelection, isInputContainsOnlyStartOfSelection, isInputContainsWholeSelection, - isInputInBetweenSelection -} from './selectionRangeInInput.js'; - -jest.mock('./getAbsoluteRangeOffset.js', () => ({ - getAbsoluteRangeOffset: jest.fn(), -})); + isInputInBetweenSelection, +} = await import('./selectionRangeInInput.js'); const mockedGetAbsoluteRangeOffset = jest.mocked(getAbsoluteRangeOffset); diff --git a/packages/dom-adapters/src/utils/useSelectionChange.spec.ts b/packages/dom-adapters/src/utils/useSelectionChange.spec.ts new file mode 100644 index 00000000..b855a8a9 --- /dev/null +++ b/packages/dom-adapters/src/utils/useSelectionChange.spec.ts @@ -0,0 +1,222 @@ +import { beforeEach, describe, expect, it, jest } from '@jest/globals'; +import { useSelectionChange } from './useSelectionChange.js'; + +/** + * Stand-in for `document`: the watcher only registers its `selectionchange` listener on it. + * It touches `document` lazily, on the first subscription, so assigning it after the import is enough + */ +const fakeDocument = { + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + getSelection: jest.fn<() => Selection | null>(() => null), +}; + +/** + * Stand-in for the `Document` class: the watcher only handles events whose target is a Document + */ +class FakeDocumentClass {} + +Object.assign(globalThis, { + document: fakeDocument, + // eslint-disable-next-line @typescript-eslint/naming-convention -- mirrors the global it stands in for + Document: FakeDocumentClass, +}); + +/** + * Builds a selection whose range intersects the given inputs only + * @param inputs - inputs the selection is inside + */ +function selectionIn(...inputs: HTMLElement[]): Selection { + return { + rangeCount: 1, + getRangeAt: () => ({ intersectsNode: (node: Node) => inputs.includes(node as HTMLElement) }), + } as unknown as Selection; +} + +/** + * Sets the document selection and fires `selectionchange` through the active document listener + * @param selection - selection the document reports + * @param target - event target, the document by default + */ +function changeSelection(selection: Selection | null, target: unknown = new FakeDocumentClass()): void { + fakeDocument.getSelection.mockReturnValue(selection); + + fakeDocument.addEventListener.mock.calls + .filter(call => call[0] === 'selectionchange' && (call[2] as AddEventListenerOptions).signal?.aborted !== true) + .forEach(call => (call[1] as (event: Event) => void)({ target } as unknown as Event)); +} + +/** + * Signals passed with every `selectionchange` registration so far, oldest first + */ +function registrationSignals(): (AbortSignal | undefined)[] { + return fakeDocument.addEventListener.mock.calls + .filter(([type]) => type === 'selectionchange') + .map(call => (call[2] as AddEventListenerOptions | undefined)?.signal); +} + +/** + * Number of `selectionchange` listeners still registered: those whose signal hasn't aborted + */ +function activeListeners(): number { + return registrationSignals().filter(signal => signal?.aborted !== true).length; +} + +describe('useSelectionChange', () => { + const watcher = useSelectionChange(); + const inputA = {} as HTMLElement; + const inputB = {} as HTMLElement; + + beforeEach(() => { + watcher.off(inputA); + watcher.off(inputB); + fakeDocument.addEventListener.mockClear(); + fakeDocument.removeEventListener.mockClear(); + }); + + it('should register one document listener, with a signal, when the first subscriber is added', () => { + watcher.on(inputA, jest.fn(), null); + watcher.on(inputB, jest.fn(), null); + + expect(registrationSignals()).toEqual([expect.any(AbortSignal)]); + expect(activeListeners()).toBe(1); + }); + + it('should keep the document listener while a subscriber is left', () => { + watcher.on(inputA, jest.fn(), null); + watcher.on(inputB, jest.fn(), null); + + watcher.off(inputA); + + expect(activeListeners()).toBe(1); + }); + + it('should release the document listener when the last subscriber unsubscribes', () => { + watcher.on(inputA, jest.fn(), null); + watcher.on(inputB, jest.fn(), null); + + watcher.off(inputA); + watcher.off(inputB); + + expect(activeListeners()).toBe(0); + expect(fakeDocument.removeEventListener).not.toHaveBeenCalled(); + }); + + it('should register a fresh document listener when a subscriber is added after all were removed', () => { + watcher.on(inputA, jest.fn(), null); + watcher.off(inputA); + + watcher.on(inputB, jest.fn(), null); + + expect(registrationSignals()).toHaveLength(2); + expect(activeListeners()).toBe(1); + }); + + it('should unsubscribe every input and release the document listener on destroy', () => { + watcher.on(inputA, jest.fn(), null); + watcher.on(inputB, jest.fn(), null); + + watcher.destroy(); + + expect(activeListeners()).toBe(0); + + watcher.on(inputA, jest.fn(), null); + + expect(activeListeners()).toBe(1); + }); + + it('should ignore off() for an input that is not subscribed', () => { + watcher.on(inputA, jest.fn(), null); + + watcher.off(inputB); + + expect(activeListeners()).toBe(1); + + watcher.off(inputA); + + expect(activeListeners()).toBe(0); + }); + + it('should pass the selection to the subscriber whose input contains it, with its context', () => { + const context = {}; + const callback = jest.fn(function (this: unknown) { + expect(this).toBe(context); + }); + const selection = selectionIn(inputA); + + watcher.on(inputA, callback, context); + changeSelection(selection); + + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(selection); + }); + + it('should pass null to the subscribers whose inputs do not contain the selection', () => { + const callbackA = jest.fn(); + const callbackB = jest.fn(); + const selection = selectionIn(inputA); + + watcher.on(inputA, callbackA, null); + watcher.on(inputB, callbackB, null); + changeSelection(selection); + + expect(callbackA).toHaveBeenCalledWith(selection); + expect(callbackB).toHaveBeenCalledWith(null); + }); + + it('should pass null to the previous input and the selection to the new one when the selection moves', () => { + const callbackA = jest.fn(); + const callbackB = jest.fn(); + + watcher.on(inputA, callbackA, null); + watcher.on(inputB, callbackB, null); + changeSelection(selectionIn(inputA)); + callbackA.mockClear(); + callbackB.mockClear(); + + const moved = selectionIn(inputB); + + changeSelection(moved); + + expect(callbackA).toHaveBeenCalledWith(null); + expect(callbackB).toHaveBeenCalledWith(moved); + }); + + it('should pass null to every subscriber when there is no selection or it has no ranges', () => { + const callback = jest.fn(); + + watcher.on(inputA, callback, null); + changeSelection(null); + changeSelection({ rangeCount: 0 } as Selection); + + expect(callback.mock.calls).toEqual([[null], [null]]); + }); + + it('should ignore selection changes that do not come from the document', () => { + const callback = jest.fn(); + + watcher.on(inputA, callback, null); + changeSelection(selectionIn(inputA), {}); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('should stop notifying a subscriber once it unsubscribes', () => { + const callbackA = jest.fn(); + const callbackB = jest.fn(); + + watcher.on(inputA, callbackA, null); + watcher.on(inputB, callbackB, null); + watcher.off(inputA); + changeSelection(selectionIn(inputA)); + + expect(callbackA).not.toHaveBeenCalled(); + expect(callbackB).toHaveBeenCalledWith(null); + }); + + it('should throw when an input is subscribed twice', () => { + watcher.on(inputA, jest.fn(), null); + + expect(() => watcher.on(inputA, jest.fn(), null)).toThrow('Input is already subscribed'); + }); +}); diff --git a/packages/dom-adapters/src/utils/useSelectionChange.ts b/packages/dom-adapters/src/utils/useSelectionChange.ts index 4ab2c0a1..4647c69c 100644 --- a/packages/dom-adapters/src/utils/useSelectionChange.ts +++ b/packages/dom-adapters/src/utils/useSelectionChange.ts @@ -29,20 +29,9 @@ export type InputWithCaret = HTMLElement; */ export const useSelectionChange = createSingleton(() => { /** - * Stores the last input that was related to the selection. - * We need that to send update when selection is moved to another input. + * Subscriber of each watched input, in subscription order */ - let lastRelatedInput: HTMLElement | null = null; - - /** - * Used to iterate over all inputs and check if selection is related to them. - */ - const inputsWatched: InputWithCaret[] = []; - - /** - * WeakMap that stores subscribers for each input. - */ - const subscribers = new WeakMap(); + const subscribers = new Map(); /** * Checks if selection is related to input @@ -79,36 +68,43 @@ export const useSelectionChange = createSingleton(() => { const selection = document.getSelection(); /** - * Iterate over all subscribers WeakMap and call their callbacks. + * Each subscriber gets the selection while it is inside its input, and null otherwise, + * so an input the selection has moved away from learns it lost it */ - inputsWatched.forEach((input) => { - const subscriber = subscribers.get(input); - - let isRelatedInputFound = false; - - if (subscriber && isSelectionRelatedToInput(selection, input)) { - lastRelatedInput = input; - isRelatedInputFound = true; - - subscriber.callback.call(subscriber.context, selection); - } - - /** - * If no related input found or input is changed, we need to notify subscriber selection is out of it's input - */ - if (!isRelatedInputFound || input !== lastRelatedInput) { - subscriber?.callback.call(subscriber.context, null); - } - - /** - * If no related input found, we need to reset last related input - */ - if (!isRelatedInputFound) { - lastRelatedInput = null; - } + subscribers.forEach((subscriber, input) => { + subscriber.callback.call(subscriber.context, isSelectionRelatedToInput(selection, input) ? selection : null); }); } + /** + * Controls the single document listener; null while no input is subscribed + */ + let documentListener: AbortController | null = null; + + /** + * Registers the document listener, unless it is already registered: editors on the page share it + */ + function listen(): void { + if (documentListener !== null) { + return; + } + + documentListener = new AbortController(); + + /** + * We use single for document "selection change" event and delegate the provided callbacks to subscribers. + */ + document.addEventListener('selectionchange', onDocumentSelectionChanged, { signal: documentListener.signal }); + } + + /** + * Releases the shared document listener + */ + function stopListening(): void { + documentListener?.abort(); + documentListener = null; + } + /** * Subscribe on "selection change" event. * @param input - input to watch caret change @@ -120,18 +116,12 @@ export const useSelectionChange = createSingleton(() => { throw new Error('Input is already subscribed to "selection change" event.'); } - /** - * Add input to the list of watched inputs. - */ - inputsWatched.push(input); - - /** - * Store subscription - */ subscribers.set(input, { callback, context, }); + + listen(); } /** @@ -140,40 +130,25 @@ export const useSelectionChange = createSingleton(() => { */ function off(input: InputWithCaret): void { subscribers.delete(input); - inputsWatched.splice(inputsWatched.indexOf(input), 1); - } - /** - * Initialize document selection change watcher. - */ - function init(): void { /** - * We use single for document "selection change" event and delegate the provided callbacks to subscribers. + * Editors on the page share the document listener, so it is removed with the last subscriber only */ - document.addEventListener('selectionchange', onDocumentSelectionChanged); + if (subscribers.size === 0) { + stopListening(); + } } /** - * Destroy document selection change watcher. + * Destroy document selection change watcher: unsubscribes every input, which removes the document listener */ function destroy(): void { - document.removeEventListener('selectionchange', onDocumentSelectionChanged); - - inputsWatched.forEach((input) => { - const subscriber = subscribers.get(input); - - if (subscriber) { - off(input); - } - }); + [...subscribers.keys()].forEach(input => off(input)); } - init(); - return { on, off, - init, destroy, }; }); diff --git a/packages/editorjs/e2e/README.md b/packages/editorjs/e2e/README.md index 2754bfb0..3613339b 100644 --- a/packages/editorjs/e2e/README.md +++ b/packages/editorjs/e2e/README.md @@ -42,6 +42,7 @@ document other than the default remounts with one: | `/?text=` | a single empty block — how the placeholder tests get one | | `/?text=&placeholder=` | an empty block with **no** `placeholder` option set at all | | `/?editors=2` | two editors on one page, for anything that has to stay unique per instance | +| `/?editors=0` | no editor at all, for a baseline from a page that never had one (tests mount their own through `window.EditorJS`) | The fixture also configures the editor's `placeholder`, which reaches the paragraph tool as `config.placeholder` and is exposed as `aria-placeholder`. diff --git a/packages/editorjs/e2e/fixtures/main.ts b/packages/editorjs/e2e/fixtures/main.ts index 3355a248..3bc39ef4 100644 --- a/packages/editorjs/e2e/fixtures/main.ts +++ b/packages/editorjs/e2e/fixtures/main.ts @@ -23,25 +23,26 @@ const placeholder = params.get('placeholder') ?? DEFAULT_PLACEHOLDER; /** * `?editors=2` mounts a second editor on the same page, the way a host with two fields would. * Anything the editor puts a document-wide `id` on has to stay unique across instances, so this - * is what makes that testable — the default stays a single editor. + * is what makes that testable — the default stays a single editor. `?editors=0` mounts none, + * for tests that need a page which never had an editor (e.g. a listener baseline). * - * Anything that isn't a positive whole number is rejected rather than coerced: `Number('abc')` + * Anything that isn't a non-negative whole number is rejected rather than coerced: `Number('abc')` * is `NaN`, which every comparison below is false against, so a typo would silently mount one * editor and let a two-editor test pass without ever having had two */ const requestedEditors = Number(params.get('editors') ?? 1); -if (!Number.isInteger(requestedEditors) || requestedEditors < 1) { +if (!Number.isInteger(requestedEditors) || requestedEditors < 0) { /** * Recorded on the body as well as thrown: nothing else reads a module-level throw, and a * spec would otherwise just time out waiting for `data-editor-ready` with no clue why */ - document.body.dataset.editorError = `"editors" must be a positive whole number, got "${params.get('editors')}"`; + document.body.dataset.editorError = `"editors" must be a non-negative whole number, got "${params.get('editors')}"`; throw new Error(document.body.dataset.editorError); } -const holders = [document.getElementById('editorjs') as HTMLElement]; +const holders = requestedEditors > 0 ? [document.getElementById('editorjs') as HTMLElement] : []; for (let index = 1; index < requestedEditors; index++) { const extraHolder = document.createElement('div'); @@ -62,6 +63,16 @@ const editors = holders.map(holder => new EditorJS({ }, })); +/** + * Exposed so tests can mount and destroy editors at moments they choose (e.g. before `isReady`), + * which the URL params can't express + */ +Object.assign(window, { + // eslint-disable-next-line @typescript-eslint/naming-convention -- exposes the class under its own name + EditorJS, + editors, +}); + Promise.all(editors.map(editor => editor.isReady)) .then(() => { document.body.dataset.editorReady = 'true'; diff --git a/packages/editorjs/e2e/support/editor.ts b/packages/editorjs/e2e/support/editor.ts index 7ed9ced3..7de8b0d0 100644 --- a/packages/editorjs/e2e/support/editor.ts +++ b/packages/editorjs/e2e/support/editor.ts @@ -26,7 +26,8 @@ export const PLACEHOLDER = DEFAULT_PLACEHOLDER; */ export async function mountDocument(page: Page, search = ''): Promise { await page.goto(`/${search}`); - await page.waitForSelector('body[data-editor-ready="true"]'); + // `attached`, not the default `visible`: with `?editors=0` the body is empty and has no size + await page.waitForSelector('body[data-editor-ready="true"]', { state: 'attached' }); } /** diff --git a/packages/editorjs/e2e/support/listeners.ts b/packages/editorjs/e2e/support/listeners.ts new file mode 100644 index 00000000..ec099ccd --- /dev/null +++ b/packages/editorjs/e2e/support/listeners.ts @@ -0,0 +1,133 @@ +import type { Page } from '@playwright/test'; + +/** + * Where to count listeners: on `document` and `window`, or on a holder element and everything inside it + */ +export type ListenerScope = { + /** + * Count listeners on `document` and `window` + */ + global: true; +} | { + /** + * Count listeners on the element matching this selector and everything inside it + */ + holderSelector: string; +}; + +/** + * What {@link trackListeners} installs on `window` + */ +interface TrackedWindow { + /** + * Lists active listeners on `document`/`window`, or inside the element matching `selector` + */ + activeListeners(selector?: string): string[]; +} + +/** + * Records every listener added through `EventTarget.prototype.addEventListener` and forgets it again on + * `removeEventListener` or when its `signal` aborts, so a test can count the listeners still active. + * + * Must run before the page's scripts, so it is installed as an init script. + * @param page - Playwright page to instrument, before navigating + */ +export async function trackListeners(page: Page): Promise { + await page.addInitScript(() => { + /** + * One active listener, keyed the way the DOM dedupes them + */ + interface ListenerRecord { + /** + * Object the listener is registered on + */ + target: EventTarget; + + /** + * Event type + */ + type: string; + + /** + * Registered callback + */ + listener: EventListenerOrEventListenerObject; + + /** + * Whether it was registered for the capture phase + */ + capture: boolean; + } + + const active: ListenerRecord[] = []; + const nativeAdd = EventTarget.prototype.addEventListener; + const nativeRemove = EventTarget.prototype.removeEventListener; + const isCapture = (options?: boolean | EventListenerOptions): boolean => + typeof options === 'boolean' ? options : options?.capture === true; + const indexOf = (target: EventTarget, type: string, listener: EventListenerOrEventListenerObject, capture: boolean): number => + active.findIndex(record => record.target === target && record.type === type && record.listener === listener && record.capture === capture); + + EventTarget.prototype.addEventListener = function (type, listener, options) { + nativeAdd.call(this, type, listener, options); + + const signal = typeof options === 'object' ? options.signal : undefined; + const capture = isCapture(options); + + if (listener === null || signal?.aborted === true || indexOf(this, type, listener, capture) !== -1) { + return; + } + + const record = { target: this, + type, + listener, + capture }; + + active.push(record); + + signal !== undefined && nativeAdd.call(signal, 'abort', () => { + const index = active.indexOf(record); + + index !== -1 && active.splice(index, 1); + }); + }; + + EventTarget.prototype.removeEventListener = function (type, listener, options) { + nativeRemove.call(this, type, listener, options); + + if (listener === null) { + return; + } + + const index = indexOf(this, type, listener, isCapture(options)); + + index !== -1 && active.splice(index, 1); + }; + + Object.assign(window, { + activeListeners: (holderSelector?: string): string[] => { + const holder = holderSelector === undefined ? null : document.querySelector(holderSelector); + + return active + .filter(({ target }) => holderSelector === undefined + ? target === document || target === window + : target instanceof Node && holder !== null && holder.contains(target)) + .map(({ target, type }) => `${target === window ? 'window' : target === document ? 'document' : (target as Element).tagName}:${type}`) + .sort(); + }, + }); + }); +} + +/** + * Lists the listeners still active in `scope`, as `target:type` strings, sorted so lists compare stably + * @param page - instrumented Playwright page + * @param scope - `document`/`window`, or a holder element and its subtree + */ +export async function activeListeners(page: Page, scope: ListenerScope): Promise { + const holderSelector = 'holderSelector' in scope ? scope.holderSelector : undefined; + + return page.evaluate( + selector => (window as unknown as TrackedWindow).activeListeners(selector), + holderSelector + ); +} diff --git a/packages/editorjs/e2e/tests/destroy.spec.ts b/packages/editorjs/e2e/tests/destroy.spec.ts new file mode 100644 index 00000000..38264cab --- /dev/null +++ b/packages/editorjs/e2e/tests/destroy.spec.ts @@ -0,0 +1,220 @@ +import { test, expect } from '@playwright/test'; +import type { Page } from '@playwright/test'; +import { mountDocument } from '../support/editor.js'; +import { activeListeners, trackListeners } from '../support/listeners.js'; + +/** + * Milliseconds to wait for the browser to report an unhandled rejection + */ +const REPORTING_DELAY = 100; + +/** + * The slice of `EditorJS` the tests drive from inside the page + */ +interface PageEditor { + /** + * Settles when initialization finishes + */ + isReady: Promise; + + /** + * Tears the editor down + */ + destroy(): void; +} + +/** + * Globals the fixture exposes (see e2e/fixtures/main.ts), plus the editors these tests mount + */ +interface FixtureWindow { + /** + * The bundle's editor class + */ + // eslint-disable-next-line @typescript-eslint/naming-convention -- mirrors the class the fixture exposes + EditorJS: new (config: Record) => PageEditor; + + /** + * Editors mounted by {@link mountExtraEditors} + */ + mounted: PageEditor[]; +} + +/** + * Mounts editors in new holders `#extra-0`, `#extra-1`, … next to the fixture's own editor and waits until they're ready + * @param page - Playwright page with the fixture loaded + * @param count - number of editors to mount + * @param config - extra editor config, e.g. `collaborationServer` + */ +async function mountExtraEditors(page: Page, count: number, config: Record = {}): Promise { + await page.evaluate(async ({ count: total, config: extra }) => { + const fixture = window as unknown as FixtureWindow; + + fixture.mounted = Array.from({ length: total }, (_, index) => { + const holder = document.createElement('div'); + + holder.id = `extra-${index}`; + document.body.appendChild(holder); + + return new fixture.EditorJS({ + holder, + data: { blocks: [{ type: 'paragraph', + data: { text: `Extra ${index}` } }] }, + ...extra, + }); + }); + + await Promise.all(fixture.mounted.map(editor => editor.isReady)); + }, { count, + config }); +} + +/** + * Destroys every editor mounted by {@link mountExtraEditors} + * @param page - Playwright page the editors are mounted on + */ +async function destroyExtraEditors(page: Page): Promise { + await page.evaluate(() => { + (window as unknown as FixtureWindow).mounted.forEach(editor => editor.destroy()); + }); +} + +test.beforeEach(async ({ page }) => { + await trackListeners(page); + await mountDocument(page); +}); + +test('removes the document and window listeners of destroyed editors', async ({ page }) => { + // Baseline from a page that never had an editor: with one alive, listeners shared between + // editors (the document selectionchange watcher) are already in the baseline and a leak of + // them would go unnoticed + await mountDocument(page, '?editors=0'); + + const before = await activeListeners(page, { global: true }); + + await mountExtraEditors(page, 2); + await destroyExtraEditors(page); + + expect(await activeListeners(page, { global: true })).toEqual(before); +}); + +test('leaves no hint tooltip or its listeners behind once a hint has been shown', async ({ page }) => { + // Same baseline as above. Showing a hint creates ui-kit's page-wide tooltip, which used to keep + // its element in and a window scroll listener for the life of the page + await mountDocument(page, '?editors=0'); + + const before = await activeListeners(page, { global: true }); + + await mountExtraEditors(page, 1); + await page.locator('#extra-0').getByRole('textbox', { name: 'Paragraph' }) + .click({ clickCount: 3 }); + await page.locator('#extra-0').getByRole('button', { name: /bold/i }) + .hover(); + await expect(page.locator('.ct--shown')).toHaveCount(1); + + await destroyExtraEditors(page); + + // The tooltip lets its hiding transition play before it releases the element + await expect(page.locator('.ct')).toHaveCount(0); + expect(await activeListeners(page, { global: true })).toEqual(before); +}); + +test('keeps the other editors on the page working after one is destroyed', async ({ page }) => { + await mountExtraEditors(page, 2); + await destroyExtraEditors(page); + + const paragraph = page.locator('#editorjs').getByRole('textbox', { name: 'Paragraph' }); + + await paragraph.click(); + await page.keyboard.press('End'); + await page.keyboard.type('!'); + + await expect(paragraph).toHaveText('Hello world!'); +}); + +test('leaves destroyed editors\' holders empty, with no listeners inside', async ({ page }) => { + await mountExtraEditors(page, 2); + await destroyExtraEditors(page); + + for (const holderSelector of ['#extra-0', '#extra-1']) { + await expect(page.locator(holderSelector)).toBeEmpty(); + expect(await activeListeners(page, { holderSelector })).toEqual([]); + } +}); + +test('rejects isReady with an AbortError when destroyed before it is ready', async ({ page }) => { + const rejection = await page.evaluate(async () => { + const fixture = window as unknown as FixtureWindow; + const holder = document.createElement('div'); + + document.body.appendChild(holder); + + const editor = new fixture.EditorJS({ holder }); + + editor.destroy(); + + return editor.isReady.then(() => 'resolved', (error: unknown) => (error as Error).name); + }); + + expect(rejection).toBe('AbortError'); +}); + +test('does not report an unhandled rejection when destroyed before it is ready', async ({ page }) => { + const errors: string[] = []; + + page.on('pageerror', error => errors.push(error.message)); + page.on('console', (message) => { + if (message.type() === 'error') { + errors.push(message.text()); + } + }); + + await page.evaluate(async (delay) => { + const holder = document.createElement('div'); + + document.body.appendChild(holder); + new (window as unknown as FixtureWindow).EditorJS({ holder }).destroy(); + + // Give an unhandled rejection the chance to be reported + await new Promise(resolve => setTimeout(resolve, delay)); + }, REPORTING_DELAY); + + expect(errors).toEqual([]); +}); + +test('leaves the holder empty when destroyed before it is ready', async ({ page }) => { + await page.evaluate(() => { + const holder = document.createElement('div'); + + holder.id = 'early'; + document.body.appendChild(holder); + new (window as unknown as FixtureWindow).EditorJS({ holder }).destroy(); + }); + + await expect(page.locator('#early')).toBeEmpty(); +}); + +test('closes the collaboration socket on destroy', async ({ page }) => { + let connected: () => void = () => undefined; + const connection = new Promise((resolve) => { + connected = resolve; + }); + let closed: () => void = () => undefined; + const closure = new Promise((resolve) => { + closed = resolve; + }); + + // Mock server: accept the connection and record when the client closes it + await page.routeWebSocket(/^ws:\/\/collaboration\.test\//, (socket) => { + connected(); + socket.onClose(() => closed()); + }); + + // WebSocket routes only apply to pages loaded after they're registered, so reload the fixture + await mountDocument(page); + await mountExtraEditors(page, 1, { collaborationServer: 'ws://collaboration.test/document' }); + await connection; + + await destroyExtraEditors(page); + + await expect(closure).resolves.toBeUndefined(); +}); diff --git a/packages/plugins/clipboard-plugin/src/index.spec.ts b/packages/plugins/clipboard-plugin/src/index.spec.ts index 609871e4..585b13a7 100644 --- a/packages/plugins/clipboard-plugin/src/index.spec.ts +++ b/packages/plugins/clipboard-plugin/src/index.spec.ts @@ -12,7 +12,13 @@ jest.unstable_mockModule('@editorjs/sdk', () => { const listeners = new Map(); return { - addEventListener: jest.fn((event: string, fn: Listener) => listeners.set(event, fn)), + /** + * Honours `signal` like a real EventTarget, so aborting it removes the listener + */ + addEventListener: jest.fn((event: string, fn: Listener, options?: AddEventListenerOptions) => { + listeners.set(event, fn); + options?.signal?.addEventListener('abort', () => listeners.delete(event), { once: true }); + }), removeEventListener: jest.fn((event: string) => listeners.delete(event)), __fire: (event: string, evt: Event) => listeners.get(event)?.(evt), }; @@ -46,18 +52,25 @@ describe('ClipboardPlugin', () => { new ClipboardPlugin(pluginParamsMock); - expect(eventBus.addEventListener).toHaveBeenCalledWith(`ui:${CopyUIEventName}`, expect.any(Function)); + expect(eventBus.addEventListener).toHaveBeenCalledWith(`ui:${CopyUIEventName}`, expect.any(Function), { signal: expect.any(AbortSignal) as unknown }); }); }); describe('.destroy()', () => { - it('should remove CopyUIEvent listener', () => { - const { eventBus } = pluginParamsMock; + it('should stop handling CopyUIEvent after destroy', () => { + const selection = pluginParamsMock.api.selection as unknown as { selectedBlocks: unknown[] }; const cp = new ClipboardPlugin(pluginParamsMock); + const preventDefault = jest.fn(); + selection.selectedBlocks = [{}]; cp.destroy(); - expect(eventBus.removeEventListener).toHaveBeenCalledWith(`ui:${CopyUIEventName}`, expect.any(Function)); + (pluginParamsMock.eventBus as unknown as FireableEventBus).__fire( + `ui:${CopyUIEventName}`, + { detail: { nativeEvent: { preventDefault } } } as unknown as Event + ); + + expect(preventDefault).not.toHaveBeenCalled(); }); }); diff --git a/packages/plugins/clipboard-plugin/src/index.ts b/packages/plugins/clipboard-plugin/src/index.ts index abfcc9d6..371ca37d 100644 --- a/packages/plugins/clipboard-plugin/src/index.ts +++ b/packages/plugins/clipboard-plugin/src/index.ts @@ -1,4 +1,4 @@ -import type { BlockData, CopyUIEvent, EditorAPI, EditorjsPlugin, EditorjsPluginParams, EventBus } from '@editorjs/sdk'; +import type { BlockData, CopyUIEvent, EditorAPI, EditorjsPlugin, EditorjsPluginParams } from '@editorjs/sdk'; import { CopyUIEventName } from '@editorjs/sdk'; import { PluginType } from '@editorjs/sdk'; @@ -31,8 +31,11 @@ export class ClipboardPlugin implements EditorjsPlugin { public static readonly name = 'clipboard'; readonly #api: EditorAPI; - readonly #eventBus: EventBus; - #copyEventListener: ((e: CopyUIEvent) => void) | undefined; + + /** + * Releases the plugin's listeners on destroy + */ + readonly #controller = new AbortController(); /** * @param params - plugin configuration and dependencies @@ -44,9 +47,8 @@ export class ClipboardPlugin implements EditorjsPlugin { const { api, eventBus } = params; this.#api = api; - this.#eventBus = eventBus; - this.#copyEventListener = (e: CopyUIEvent) => { + eventBus.addEventListener(`ui:${CopyUIEventName}`, (e: CopyUIEvent) => { const { nativeEvent } = e.detail; const selectedBlocks = this.#api.selection.selectedBlocks; @@ -82,27 +84,14 @@ export class ClipboardPlugin implements EditorjsPlugin { clipboardData.setData('text/plain', selectionAsPlainText); clipboardData.setData('text/html', selectionAsHTML); clipboardData.setData(EDITOR_JS_CLIPBOARD_MIME_TYPE, JSON.stringify(clipboardEditorJSObject)); - }; - - eventBus.addEventListener(`ui:${CopyUIEventName}`, this.#copyEventListener); + }, { signal: this.#controller.signal }); } /** * Destroys the plugin and removes all event listeners */ public destroy(): void { - this.#removeEventListener(); - } - - /** - * Removes the event listener for copy events - * @internal - */ - #removeEventListener(): void { - if (this.#copyEventListener !== undefined) { - this.#eventBus.removeEventListener(`ui:${CopyUIEventName}`, this.#copyEventListener); - } - this.#copyEventListener = undefined; + this.#controller.abort(); } /** diff --git a/packages/plugins/shortcuts-plugin/src/index.spec.ts b/packages/plugins/shortcuts-plugin/src/index.spec.ts index 74d5d4f3..6ee3779f 100644 --- a/packages/plugins/shortcuts-plugin/src/index.spec.ts +++ b/packages/plugins/shortcuts-plugin/src/index.spec.ts @@ -23,7 +23,13 @@ jest.unstable_mockModule('@editorjs/sdk', () => { const listeners = new Map(); return { - addEventListener: jest.fn((event: string, fn: Listener) => listeners.set(event, fn)), + /** + * Honours `signal` like a real EventTarget, so aborting it removes the listener + */ + addEventListener: jest.fn((event: string, fn: Listener, options?: AddEventListenerOptions) => { + listeners.set(event, fn); + options?.signal?.addEventListener('abort', () => listeners.delete(event), { once: true }); + }), removeEventListener: jest.fn((event: string) => listeners.delete(event)), __fire: (event: string, evt: Event) => listeners.get(event)?.(evt), }; @@ -91,13 +97,13 @@ describe('ShortcutsPlugin', () => { it('should add tool-loaded listener', () => { new ShortcutsPlugin(pluginParamsMock); - expect(pluginParamsMock.eventBus.addEventListener).toHaveBeenCalledWith(TOOL_LOADED_EVENT, expect.any(Function)); + expect(pluginParamsMock.eventBus.addEventListener).toHaveBeenCalledWith(TOOL_LOADED_EVENT, expect.any(Function), { signal: expect.any(AbortSignal) as unknown }); }); it('should add keydown listener', () => { new ShortcutsPlugin(pluginParamsMock); - expect(pluginParamsMock.eventBus.addEventListener).toHaveBeenCalledWith(KEYDOWN_EVENT, expect.any(Function)); + expect(pluginParamsMock.eventBus.addEventListener).toHaveBeenCalledWith(KEYDOWN_EVENT, expect.any(Function), { signal: expect.any(AbortSignal) as unknown }); }); }); @@ -211,6 +217,32 @@ describe('ShortcutsPlugin', () => { }); describe('.destroy()', () => { + it('should stop registering shortcuts of tools loaded after destroy', () => { + const plugin = new ShortcutsPlugin(pluginParamsMock); + + plugin.destroy(); + loadTool('bold', { shortcut: 'CMD+B' }); + matchKeyboardShortcut.mockReturnValue(true); + + pressKey(); + + expect(applyInlineTool).not.toHaveBeenCalled(); + }); + + it('should stop handling keydown after destroy', () => { + const plugin = new ShortcutsPlugin(pluginParamsMock); + const handler = jest.fn(); + + plugin.publicApi.register('CMD+K', handler); + plugin.destroy(); + plugin.publicApi.register('CMD+K', handler); + matchKeyboardShortcut.mockReturnValue(true); + + pressKey(); + + expect(handler).not.toHaveBeenCalled(); + }); + it('should clear registered shortcuts so later keydowns dispatch nothing', () => { const plugin = new ShortcutsPlugin(pluginParamsMock); diff --git a/packages/plugins/shortcuts-plugin/src/index.ts b/packages/plugins/shortcuts-plugin/src/index.ts index a222cea3..c927399e 100644 --- a/packages/plugins/shortcuts-plugin/src/index.ts +++ b/packages/plugins/shortcuts-plugin/src/index.ts @@ -114,6 +114,11 @@ export class ShortcutsPlugin implements EditorjsPlugin<'shortcuts'> { }, }; + /** + * Releases the plugin's listeners on destroy + */ + readonly #controller = new AbortController(); + /** * @param params - {@link EditorjsPluginParams} */ @@ -122,6 +127,8 @@ export class ShortcutsPlugin implements EditorjsPlugin<'shortcuts'> { this.#api = api; + const { signal } = this.#controller; + eventBus.addEventListener(`core:${CoreEventType.ToolLoaded}`, (event: Event) => { const { detail } = event as ToolLoadedCoreEvent; const { tool } = detail; @@ -150,7 +157,7 @@ export class ShortcutsPlugin implements EditorjsPlugin<'shortcuts'> { * }, * }) */ - }); + }, { signal }); eventBus.addEventListener(`ui:${KeydownUIEventName}`, (event: Event) => { const { detail } = event as KeydownUIEvent; @@ -169,13 +176,14 @@ export class ShortcutsPlugin implements EditorjsPlugin<'shortcuts'> { return; } } - }); + }, { signal }); } /** * Destroys the plugin */ public destroy(): void { + this.#controller.abort(); this.#handlers.clear(); } diff --git a/packages/sdk/package.json b/packages/sdk/package.json index de94fe26..24e3dc53 100644 --- a/packages/sdk/package.json +++ b/packages/sdk/package.json @@ -33,7 +33,7 @@ "dependencies": { "@editorjs/helpers": "^1.2.2", "@editorjs/model-types": "workspace:^", - "@editorjs/ui-kit": "^2.0.0", + "@editorjs/ui-kit": "^2.0.1", "editorjs-v2": "npm:@editorjs/editorjs@^2.30.8" } } diff --git a/packages/sdk/src/entities/EditorjsAdapterPlugin.ts b/packages/sdk/src/entities/EditorjsAdapterPlugin.ts index 163881f2..46cfc262 100644 --- a/packages/sdk/src/entities/EditorjsAdapterPlugin.ts +++ b/packages/sdk/src/entities/EditorjsAdapterPlugin.ts @@ -23,6 +23,12 @@ export interface EditorJSAdapterPlugin extends E * @param blockId - unique identifier of the removed block */ destroyBlockToolAdapter(blockId: BlockId): void; + + /** + * Destroys every block adapter still alive and releases the adapter's own subscriptions. + * Called when the editor is destroyed, after every rendered block has been released + */ + destroy(): void; } /** diff --git a/packages/ui/package.json b/packages/ui/package.json index 3bf8e34f..e0782440 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -57,6 +57,6 @@ "@editorjs/dom": "^1.2.0", "@editorjs/helpers": "^1.2.2", "@editorjs/sdk": "workspace:^", - "@editorjs/ui-kit": "^2.0.0" + "@editorjs/ui-kit": "^2.0.1" } } diff --git a/packages/ui/src/lifecycle.spec.ts b/packages/ui/src/lifecycle.spec.ts index 72f54330..87612bdc 100644 --- a/packages/ui/src/lifecycle.spec.ts +++ b/packages/ui/src/lifecycle.spec.ts @@ -1,8 +1,8 @@ /** @jest-environment jsdom */ // eslint-disable-line jsdoc/check-tag-names -- Jest directive, not an API comment. /* eslint-disable jsdoc/require-jsdoc, @typescript-eslint/naming-convention */ import { afterEach, beforeEach, describe, expect, it, jest } from '@jest/globals'; -import { BlockAddedCoreEvent, BlockRemovedCoreEvent, CoreEventType, createDataKey, EventBus, TextIndex } from '@editorjs/sdk'; -import type { EditorjsPluginParams } from '@editorjs/sdk'; +import { BlockAddedCoreEvent, BlockRemovedCoreEvent, CoreEventType, createDataKey, EventBus, InlineToolFacade, TextIndex, ToolType } from '@editorjs/sdk'; +import type { EditorAPI, EditorjsPluginParams, InlineToolConstructor } from '@editorjs/sdk'; interface Item { onActivate?: () => void; @@ -93,7 +93,7 @@ describe('UI plugin listener lifetime', () => { return block; } - function selection(getToolbarConfig: () => unknown): void { + function selection(getToolbarConfig: () => unknown, availableInlineTools?: unknown[]): void { const text = document.createTextNode('hello'); const range = document.createRange(); @@ -114,7 +114,7 @@ describe('UI plugin listener lifetime', () => { dataKey: createDataKey('text'), textRange: [0, 2] }]), fragments: [], - availableInlineTools: [{ + availableInlineTools: availableInlineTools ?? [{ name: 'bold', options: {}, create: () => ({ getToolbarConfig, @@ -415,4 +415,72 @@ describe('UI plugin listener lifetime', () => { expect(() => plugin.destroy()).not.toThrow(); expect(TestPopover.instances.map(popover => popover.destroy.mock.calls.length)).toEqual(popoverDestroys); }); + + it('should never destroy the shared inline tool instance it renders, through the real facade', async () => { + const destroy = jest.fn(); + const constructed = jest.fn(); + const config = (): { type: string; + icon: string; } => ({ type: 'default', + icon: 'B' }); + + /** + * Inline tool built by the real facade, which hands the same instance to every consumer + */ + class SharedInlineTool { + public static type = ToolType.Inline; + + constructor() { + constructed(); + } + + public getToolbarConfig(): { type: string; + icon: string; } { + return config(); + } + + public isActive(): boolean { + return false; + } + + public destroy(): void { + destroy(); + } + } + + const facade = new InlineToolFacade({ + api: {} as EditorAPI, + constructable: SharedInlineTool as unknown as InlineToolConstructor, + isDefault: false, + name: 'bold', + useToolOptions: {}, + }); + const toolbar = retain(new InlineToolbarUI(params)); + let resolveConfig!: () => void; + const pending = new Promise((resolve) => { + resolveConfig = resolve; + }); + + // Rebuild for a new selection + selection(config, [facade]); + await flushRender(); + selection(config, [facade]); + await flushRender(); + + // Hide + emit(`core:${CoreEventType.SelectionChanged}`, { index: undefined, + fragments: [], + availableInlineTools: [] }); + + // A render superseded before it finishes + jest.spyOn(SharedInlineTool.prototype, 'getToolbarConfig').mockImplementationOnce(() => pending.then(config) as never); + selection(config, [facade]); + selection(config, [facade]); + resolveConfig(); + await flushRender(); + + toolbar.destroy(); + + expect(constructed).toHaveBeenCalledTimes(1); + expect(destroy).not.toHaveBeenCalled(); + }); }); diff --git a/yarn.lock b/yarn.lock index 05b73a8e..20cf99fa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2506,6 +2506,15 @@ __metadata: languageName: node linkType: hard +"@editorjs/helpers@npm:^1.2.3": + version: 1.2.3 + resolution: "@editorjs/helpers@npm:1.2.3" + dependencies: + codex-tooltip: "npm:^1.1.0" + checksum: 9e7d875fbfe120b5a9c20b7075b35f128e7edf35b5875e8bad4e825b3521d9a91b66330c29d36f193f8c6ad489daed05f4be8855cab86435d462c8db7f59d586 + languageName: node + linkType: hard + "@editorjs/inline-link@workspace:^, @editorjs/inline-link@workspace:packages/tools/inline-link": version: 0.0.0-use.local resolution: "@editorjs/inline-link@workspace:packages/tools/inline-link" @@ -2630,7 +2639,7 @@ __metadata: "@babel/preset-env": "npm:^7.29.2" "@editorjs/helpers": "npm:^1.2.2" "@editorjs/model-types": "workspace:^" - "@editorjs/ui-kit": "npm:^2.0.0" + "@editorjs/ui-kit": "npm:^2.0.1" "@jest/globals": "npm:^29.7.0" "@types/eslint": "npm:^9.6.1" "@types/jest": "npm:^29.5.12" @@ -2673,15 +2682,15 @@ __metadata: languageName: unknown linkType: soft -"@editorjs/ui-kit@npm:^2.0.0": - version: 2.0.0 - resolution: "@editorjs/ui-kit@npm:2.0.0" +"@editorjs/ui-kit@npm:^2.0.1": + version: 2.0.1 + resolution: "@editorjs/ui-kit@npm:2.0.1" dependencies: "@codexteam/icons": "npm:^0.3.3" "@editorjs/caret": "npm:^1.1.0" "@editorjs/dom": "npm:^1.2.0" - "@editorjs/helpers": "npm:^1.2.2" - checksum: 97110bd1d12fe22077cff40bf9d7df044d6cdc086cdb80c0bcd0b5a96b1363d7efa10cce62db67932f9daa0ebe782b44ed3f749d4f5781d703d2a75b0e7e8f74 + "@editorjs/helpers": "npm:^1.2.3" + checksum: 67938dbe86b811a32dd10660664c18277f94f6769a692335bfb4a0721da9298992f596686540bd8f5641d74ca56c615c413eb2da4239a4af74f4955e50cbc6b4 languageName: node linkType: hard @@ -2697,7 +2706,7 @@ __metadata: "@editorjs/helpers": "npm:^1.2.2" "@editorjs/paragraph": "workspace:^" "@editorjs/sdk": "workspace:^" - "@editorjs/ui-kit": "npm:^2.0.0" + "@editorjs/ui-kit": "npm:^2.0.1" "@jest/globals": "npm:^29.7.0" "@types/jest": "npm:^29.5.12" chokidar-cli: "npm:^3.0.0" @@ -6182,6 +6191,13 @@ __metadata: languageName: node linkType: hard +"codex-tooltip@npm:^1.1.0": + version: 1.1.0 + resolution: "codex-tooltip@npm:1.1.0" + checksum: c66da318e18e103daf163803ca4080ce2e3218c0939df255fe241ab3100c5d4212070825d145db652eff7cd1839ba5b8f9970f08c2718caf5005a7a9595ec62e + languageName: node + linkType: hard + "collect-v8-coverage@npm:^1.0.0": version: 1.0.3 resolution: "collect-v8-coverage@npm:1.0.3"