Skip to content

feat(core): add editor destroy lifecycle - #196

Open
gohabereg wants to merge 8 commits into
feat/editor-destroyfrom
feat/editor-destroy-lifecycle
Open

gohabereg wants to merge 8 commits into
feat/editor-destroyfrom
feat/editor-destroy-lifecycle

Conversation

@gohabereg

@gohabereg gohabereg commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Why

First implementation step of the editor-destroy OpenSpec change (spec PR #195, the base of this one). Adds the editor teardown lifecycle and the API changes it needs; the adapters/UI/plugins side follows in a stacked PR.

What Changes

  • Core#destroy(): plugins in reverse construction order (destroy?.() + PluginRegistry.unregister), then UndoRedoManager, SelectionManager, BlockRenderer, the adapter (destroy?.(), made required in the next PR along with DOMAdapters.destroy()), and last ToolsManager. Each step runs through a helper that logs and continues. Services resolved in initialize() are recorded in #services, so teardown never constructs one. A second call is a no-op because instances and services are cleared. use()/initialize() throw afterwards. Core's onModelUpdate model listener goes through a controller that destroy() aborts, so a model the integrator kept no longer reaches the callback after teardown.
  • Interrupted initialize() rejects with a DOMException named AbortError instead of booting the document or dispatching core:ready. ToolsManager owns an AbortController that its destroy() aborts. Its preparation queue checks that signal before and after each tool's prepare(). Tools still queued are never prepared, and a tool whose prepare() settles after destroy is never created, registered or announced with ToolLoaded. A prepare() already running can't be interrupted, because it takes no signal, so its result is dropped.
  • EditorJS#destroy() delegates to Core. The bundle attaches reportUnlessAborted() to its ready promise: AbortError is ignored and other init errors are logged, so nothing is reported as an unhandled rejection. Awaiting isReady still rejects.
  • BlockRenderer keeps each block's tool instance and destroys it on block removal and on teardown. A render that settles after teardown dispatches nothing.
  • Tool contracts: optional destroy() on BlockTool/InlineTool, called once per instance by its owner:
    • Block tools: BlockRenderer owns each per-block instance.
    • Inline tools: InlineToolFacade.create() returns one cached instance per tool, shared by formatting, selection, validation and the inline toolbar. None of those consumers destroys it. The facade's new destroy() does, once and only if the instance was ever created, and ToolsManager.destroy() calls it for every inline tool, logging a throwing hook and moving on to the next.
  • Listeners through AbortController: onUpdate/onCaretUpdate subscribe through their own controller (signatures unchanged). UndoRedoManager, SelectionManager and BlockRenderer abort one controller each. BlockToolAdapter owns its controller and exposes protected signal to subclasses. EditorJSModel's typed addEventListener overloads accept options.
  • OpenSpec: the spec revisions behind these fixes (D3, D4, D7, and the new core and sdk scenarios) are in docs(openspec): propose editor-destroy #195 and merged into this branch. This PR only checks off their tasks.

Verification

  • Full workspace build, lint and tests pass.
  • Mutation testing on every line this PR adds (Stryker scoped to the diff, -w): all mutants killed in core, and in sdk/editorjs via an ad-hoc run (those packages have no Stryker config). model changes are type-only. The packages' own yarn test:mutations pass.
  • Tests cover:
    • teardown order, including inline tools after the adapter; error logging; idempotency
    • destroy before and during initialization, including tools still queued and a prepare() that settles after destroy
    • late renders
    • tool instance destruction: each shared inline instance destroyed exactly once, and never by SelectionManager or ToolsManager validation
    • the AbortError handling, including that it is not left unhandled
    • onModelUpdate after destroy

Notes

  • ToolsManager's inline-tool render check is pinned by tests as-is; removing it is tracked on the board ("Remove inverted inline tool render check in ToolsManager").
  • The integration tasks (real DOM, several editors per page) are covered by the e2e suite in feat: implement destroy() in adapters and plugins #207.

Stack

  1. docs(openspec): propose editor-destroy #195 spec
  2. This PR
  3. feat: implement destroy() in adapters and plugins #207 adapters, plugins, e2e tests and the archive (stacked on this one)

🤖 Generated with Claude Code

gohabereg and others added 3 commits October 6, 2026 18:13
- Core#destroy(): destroys plugins in reverse construction order and
  unregisters their public APIs, then UndoRedoManager, SelectionManager,
  BlockRenderer and the adapter. Idempotent, safe before and during
  initialize(), never mutates the model. use()/initialize() throw after it.
- EditorJS#destroy() delegates to Core.
- BlockRenderer keeps each block's tool instance and destroys it on block
  removal and on teardown; a render settling after teardown is dropped.
- Optional destroy() on BlockTool/InlineTool; throwaway inline tool
  instances (SelectionManager, ToolsManager validation) are destroyed
  right after use.
- Listeners are released through AbortController: onUpdate/onCaretUpdate
  subscribe through their own controller, BlockToolAdapter owns one and
  exposes a protected signal to subclasses, core services abort theirs
  on destroy. Model addEventListener overloads accept options.

Part of the editor-destroy OpenSpec change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Cover teardown error logging, services that were never resolved, an
  adapter without destroy(), and blocks removed without a tracked tool.
- Cover ToolsManager inline tool validation around the destroyed
  validation instance.
- Drop the redundant destroyed guard in Core.destroy(): clearing plugin
  instances and services already makes a second call a no-op.
- Drop { once: true } on the BlockToolAdapter abort listener: a signal
  aborts only once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tError

- Core.initialize() rejects with a DOMException named AbortError when
  destroy() runs while tools are being prepared, instead of resolving.
- The EditorJS bundle handles its own ready promise: an AbortError is
  ignored, any other initialization error is logged, so neither is
  reported as an unhandled rejection. Awaiting isReady still rejects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Unit Tests

Package Coverage Delta
@editorjs/shortcuts-plugin 100% 0% ⚪️
@editorjs/collaboration-manager 85.81% 0% ⚪️
@editorjs/clipboard-plugin 66.66% 0% ⚪️
@editorjs/dom-adapters 86.95% 0% ⚪️
@editorjs/editorjs 100% 0% ⚪️
@editorjs/ot-server 20% 0% ⚪️
@editorjs/model 98.48% 0% ⚪️
@editorjs/core 77.02% +3.88% 🟢
@editorjs/model-types N/A N/A

Mutation Tests

Package Mutation score Dashboard URL
@editorjs/shortcuts-plugin No files to mutate found.
@editorjs/clipboard-plugin No files to mutate found.
@editorjs/dom-adapters No files to mutate found.
@editorjs/model NaN% Dashboard
@editorjs/collaboration-manager No files to mutate found.
@editorjs/core 74.64% 🟡 Dashboard

* The document model is left untouched, so no removal reaches undo history or collaboration.
* Safe to call more than once and while {@link initialize} is pending. After it, `use()` and `initialize()` throw
*/
public destroy(): void {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroy() never releases the onModelUpdate listener registered in the constructor.

It is this.#model.addEventListener(EventType.Changed, ...) with no signal, and teardown does not call removeEventListener for it. The other listeners in this PR are tied to an AbortController and aborted here. This one stays for as long as the model is reachable.

destroy() does not mutate the model, so the callback does not run during teardown itself. It does run on any later change: something still holding api, or a collaboration op that arrives after unmount. The integrator passed that callback and then unmounted, so it fires into a tree that is already gone.

Abort it from here, same as the service listeners.

/**
* destroy() was called while tools were being prepared: it already tore down what exists
*/
if (this.#destroyed) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check cancellation inside ToolsManager before post-prepare() validation. If destroy() runs while an inline tool is preparing, tool.create() and ToolLoaded still run after teardown, before this guard rejects readiness.

gohabereg and others added 2 commits November 4, 2026 18:38
…cade

InlineToolFacade.create() hands the same cached instance to every
consumer, but ToolsManager's validation and SelectionManager destroyed
it after each use, so a tool with a destroy() hook was torn down at
startup and after every formatting action, and then used again.

- InlineToolFacade.destroy() destroys the shared instance once, dropping
  it before the hook runs.
- ToolsManager.destroy() destroys every inline tool facade; a throwing
  hook is logged and the remaining tools are still destroyed.
- Core.destroy() calls it last, after the adapter, since the adapter and
  the UI use the instances until then.
- Validation and SelectionManager.applyInlineTool no longer destroy the
  instance.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Core registered its onModelUpdate model listener without a way to remove
it. An integrator that kept the model it was handed could still reach
the callback after teardown. The listener now uses Core's own controller,
which destroy() aborts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gohabereg and others added 2 commits November 4, 2026 23:59
The tool preparation queue kept running after destroy(). Tools still
queued were prepared, and a tool whose prepare() settled after teardown
was created, registered and announced with ToolLoaded. Its inline
instance then outlived ToolsManager.destroy().

ToolsManager now owns an AbortController that destroy() aborts. Each
queued job checks the signal before calling prepare() and again once it
settles. A prepare() already running can't be interrupted, since it
takes no signal, so its result is dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	openspec/changes/editor-destroy/tasks.md
gohabereg added a commit that referenced this pull request Nov 5, 2026
Review on #196 found that the tool preparation queue kept running after
destroy(). Tools still queued were prepared, and a tool whose prepare()
settled after teardown was created and announced. D4 now describes the
fix: ToolsManager owns an AbortController that destroy() aborts, and
each queued job checks it before and after prepare(). A prepare()
already running can't be interrupted, so its result is dropped.

The core scenario for a destroy during a pending initialize() covers
it, and task 6.5 tracks the implementation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@gohabereg
gohabereg force-pushed the feat/editor-destroy-lifecycle branch from a8c4898 to b17b8ea Compare November 5, 2026 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants