From 96114eefa60d8cc0b3e9006302c1eda65e4399a8 Mon Sep 17 00:00:00 2001 From: Reversean Date: Wed, 29 Jul 2026 01:19:48 +0300 Subject: [PATCH 1/6] docs(openspec): propose separating tool options from ToolConfig Header tool's v3 migration (editor-js/header#130) exposed a real gap: static `options` (toolbox, shortcut, etc.) and `ToolConfig` are two disconnected surfaces, so config fields like `levels` can't drive toolbox entries. This change proposes an SDK-owned ToolConfig contract and a prepare()-based mechanism to resolve config-derived options once, per tool registration. Co-Authored-By: Claude Sonnet 5 --- .../.openspec.yaml | 2 + .../design.md | 80 +++++++++++++++++++ .../proposal.md | 31 +++++++ .../specs/sdk/spec.md | 36 +++++++++ .../separate-tool-options-and-config/tasks.md | 36 +++++++++ 5 files changed, 185 insertions(+) create mode 100644 openspec/changes/separate-tool-options-and-config/.openspec.yaml create mode 100644 openspec/changes/separate-tool-options-and-config/design.md create mode 100644 openspec/changes/separate-tool-options-and-config/proposal.md create mode 100644 openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md create mode 100644 openspec/changes/separate-tool-options-and-config/tasks.md diff --git a/openspec/changes/separate-tool-options-and-config/.openspec.yaml b/openspec/changes/separate-tool-options-and-config/.openspec.yaml new file mode 100644 index 00000000..e8209ffa --- /dev/null +++ b/openspec/changes/separate-tool-options-and-config/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-28 diff --git a/openspec/changes/separate-tool-options-and-config/design.md b/openspec/changes/separate-tool-options-and-config/design.md new file mode 100644 index 00000000..35a6cc19 --- /dev/null +++ b/openspec/changes/separate-tool-options-and-config/design.md @@ -0,0 +1,80 @@ +## Context + +Today a tool's static surface is one bag (`ToolConstructor.options`) that mixes two things with different audiences and different lifetimes: + +- **Core/plugin-facing wiring** — `toolbox`, `shortcut`, `inlineToolbar`, `tunes`, `conversionConfig`, `canBeSplit`, `isReadOnlySupported`. Read by `BaseToolFacade`/`BlockToolFacade` getters and consumed by plugins (`ToolboxUI`, `ShortcutsPlugin`, inline toolbar, tunes) **before any block instance exists**. +- **Tool-author-facing user data** — `ToolConfig`, nested under `options.config`, merged with any `use()`-time `config` override in `BaseToolFacade.config`, and handed to the tool's `constructor`/`prepare()`. + +`ToolConfig` itself is not an SDK contract — it's re-exported from the legacy `@editorjs/editorjs` package as `type ToolConfig = T`, an untyped passthrough. A tool author gets whatever structure they declare in their own `Config` generic parameter, with nothing checking that it's consistent with anything else. + +The concrete failure this produces is the Header tool's v3 migration (draft PR [editor-js/header#130](https://github.com/editor-js/header/pull/130)): `HeaderConfig.levels` is declared but never read anywhere — the static `options.toolbox` array is a hardcoded 3-entry list (H1–H3) that has no way to depend on `config.levels`, because `toolbox` is evaluated once, statically, at class-definition time, while `config` is only resolved per tool-registration (in `BaseToolFacade.config`) and per block instance (in the constructor). + +The lifecycle already has a hook positioned exactly where this could be fixed: `ToolsManager.prepareTools()` calls `toolConstructor.prepare({ toolName, config: tool.config })` — with `tool.config` already the fully merged config — and only *after* that call resolves does it call `setToAvailableToolsCollection`, which dispatches `ToolLoadedCoreEvent`. `ToolboxUI` only starts reading a tool's `toolbox` getter in response to that event. So `prepare()` already runs at the right time, with the right data; it just isn't a channel that can influence `options` today, and nothing tells a tool author it's meant to be. + +Constraints this design has to respect: +- A page can run multiple `Core` instances that both `use()` the same tool class (e.g. two editors sharing one `Header` import). Anything a tool computes from its own resolved config must not be written onto the shared class/static object, or the second instance's config would clobber the first's. +- Per this change's proposal, a breaking change to `ToolConfig`'s type/import is acceptable; a live "change config after mount" API is explicitly out of scope. +- The project's existing TDD convention applies to the facade/manager changes below (see `openspec/config.yaml` rules). + +## Goals / Non-Goals + +**Goals:** +- Make `ToolConfig` an SDK-owned, real contract (not a re-exported `any`-defaulted passthrough). +- Keep `ToolOptions` (`BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions`) as the static, core/plugin-facing contract, formally distinct from `ToolConfig`. +- Let a tool compute config-derived static options (starting with `toolbox`) from its fully-resolved `ToolConfig`, resolved once during tool preparation, via the existing `prepare()` hook — without mutating the tool's shared static class property. +- Catch the `HeaderConfig.levels`-style failure mode: a config key that's declared but never actually consumed. + +**Non-Goals:** +- No live/reactive config updates after the editor has mounted (confirmed out of scope — config is resolved once, during preparation). +- No change to how `shortcut`, `inlineToolbar`, or `tunes` are merged — this change only adds the config-derived tier to `toolbox`, since that's the concrete, evidenced need; the same mechanism can be extended to other option fields later if a real case shows up. +- Not migrating the `header` tool itself — it lives in a separate repo/submodule. This change ships the SDK mechanism; wiring `Header.prepare()` to use it is a follow-up in that repo. +- No new runtime schema-validation dependency (e.g. zod/io-ts). + +## Decisions + +**1. `ToolConfig` becomes an SDK-owned type, not a re-export of `@editorjs/editorjs`'s passthrough.** +The legacy `ToolConfig = T` is how the untyped-escape-hatch problem enters v3 in the first place — any tool author who doesn't explicitly parameterize their `Config` generic silently gets `any`. SDK defines its own `ToolConfig` base (still generic per tool, but anchored in `@editorjs/sdk` so it's the type the rest of this design's checks can hook into). +*Alternative considered*: leave the re-export and only tighten the `Config extends ToolConfig` bound on each interface (`BlockToolOptions`, etc.). Rejected — the default still resolves to `any` for any tool that skips the generic, which is exactly today's failure mode. + +**2. `options` and `config` stay two separate top-level concepts; `options.config` remains the only bridge.** +This matches how they're actually consumed: `options` (via facade getters) is read by core/plugins before any block exists; `config` is read by the tool instance itself. Collapsing them into one `ToolSettings` bag would erase that timing distinction, which is the actual source of the bug (something that's plugin-timing data, like `toolbox`, has no way to see something that's tool-instance-timing data, like `config.levels`, unless the two are explicitly bridged). +*Alternative considered*: a single unified settings object passed everywhere. Rejected — every plugin consumer would need to filter out user-data noise, and it doesn't resolve the "when is this available" question. + +**3. Config-derived options are resolved via `prepare()`'s return value, held per-facade-instance — not by writing onto `ClassName.options`.** +`prepare()`'s signature extends to: +```ts +prepare?(data: { toolName: string, config: Config }): + PreparedToolOptions | void | Promise | void>; +``` +where `PreparedToolOptions` is a partial type restricted, for now, to `{ toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false }`. `ToolsManager.prepareTools()` captures this return value and passes it through `ToolsFactory` into the facade constructor as a new `preparedOptions` field, stored on the `BaseToolFacade` instance (not on `constructable`). +*Alternative considered*: have the tool mutate `ClassName.options` directly inside `prepare()` (the shape floated during earlier discussion of this change). Rejected once the multi-`Core`-instance case was worked through: `constructable.options` is one object shared by every facade wrapping that class, so a second `Core` instance's `prepare()` call would overwrite the first instance's computed toolbox for both. Returning a value and letting the facade own it keeps the result properly scoped per editor instance while still using the same trigger point (`prepare()`) already agreed on. + +**4. `toolbox` resolution gains a third tier, in this order: static default → prepared (config-derived) → explicit `use()`-time override.** +`BlockToolFacade.toolbox` keeps its existing array/object positional-merge algorithm; it's just seeded from `preparedOptions.toolbox ?? constructable.options.toolbox` instead of `constructable.options.toolbox` alone. All current explicit-override behavior (`toolbox: false` to hide, partial per-entry overrides via `use()`) is preserved unchanged, now layered on top of whatever the tool computed from its own config. +*Alternative considered*: make `preparedOptions.toolbox` and `useToolOptions.toolbox` mutually exclusive. Rejected as an unnecessary behavior cliff — keeping one uniform merge means the new tier is additive rather than a special case integrators need to remember. + +**5. Verification is a dev-time reachability check on the resolved config object, not a schema-validation library.** +A lightweight check (gated the same way existing `ToolsManager` dev diagnostics are — `console.warn`, not thrown, and skippable in production builds) flags a key present in a tool's *resolved* `ToolConfig` object that is never read: neither passed through via `options.config` defaults, nor touched by a `prepare()` that returns `PreparedToolOptions`. This is a reachability check on real objects and function calls at runtime, not a compile-time-only type comparison (TS types don't exist at runtime) — so it directly catches the `HeaderConfig.levels`-declared-but-unused case without requiring a new dependency. +*Alternative considered*: full schema validation of `ToolConfig` shapes (zod/io-ts). Rejected as disproportionate to the actual failure mode, which is "declared but never read," not "wrong shape." + +**6. Tools without a `prepare()` method are unaffected at runtime.** +`bold`, `italic`, `inline-link`, and `paragraph` don't currently need config-derived options; their migration is limited to importing the new SDK-owned `ToolConfig` type. Even though a breaking change is acceptable per this change's scope, there's no reason to force a runtime-behavior migration where only the type source moved. + +## Risks / Trade-offs + +- **[Risk]** A third merge tier (static → prepared → `use()`-override) adds a step to an already non-trivial merge chain in `BlockToolFacade.toolbox`, raising the bar for contributors reading it. → **Mitigation**: keep all three tiers resolved in one getter (as today), document them with a spec scenario (see delta spec), and add a facade unit test per tier combination (static-only, prepared-only, both, both-plus-explicit-override). +- **[Risk]** Widening `prepare()`'s return type is itself a breaking change for any hypothetical tool already using `prepare()` for pure side effects that happens to return a truthy non-`undefined` value. → **Mitigation**: only recognized keys (`toolbox`, for now) are read off the returned object; anything else is ignored with a dev-time warning rather than silently applied or throwing. +- **[Risk]** The "unused config key" check can only see what's reachable at runtime, not a tool's full declared type (erased at compile time), so it can miss a key that exists in the `Config` type but was never included in a given call's resolved config object. → **Mitigation**: scope the check's guarantee accordingly — it catches "this resolved config has a key nothing reads," which is exactly the Header failure mode, not "this type has a key that's structurally unreachable." +- **[Risk]** The validating real-world case (`editor-js/header#130`) lives outside this repo and could merge with the bug still present before this change ships its mechanism. → **Mitigation**: tasks.md treats that PR as an acceptance reference, not a task owned by this change; wiring `Header.prepare()` to the new mechanism is explicitly a follow-up in the `header` repo. + +## Migration Plan + +- Ship the SDK contract changes (`ToolConfig`, `ToolOptions`, `prepare()` signature), facade changes, and `ToolsManager`/`ToolsFactory` plumbing together — they're tightly coupled; a partial rollout would leave the new `prepare()` return type with no consumer. +- Update the four in-repo tools' `Config` type imports (mechanical, type-only, no runtime behavior change). +- No feature flag or staged rollout: this is a pre-1.0 internal SDK surface, and per this change's confirmed scope a breaking change is acceptable with no external in-repo consumers beyond the tools already covered. +- Rollback: revert the SDK/facade/core commits together; the tools' type-only import changes revert cleanly since they carry no runtime behavior. + +## Open Questions + +- Should `PreparedToolOptions` generalize beyond `toolbox` (e.g. `shortcut`, `inlineToolbar`) once a second concrete need appears, or stay toolbox-only indefinitely? Leaning toolbox-only until evidenced otherwise (YAGNI). +- Should the "unused config key" dev warning live in SDK core (always on outside production, matching existing `ToolsManager` console diagnostics) or as an opt-in lint/test helper tool authors run in CI? Leaning SDK-core for now; worth revisiting if it proves noisy. diff --git a/openspec/changes/separate-tool-options-and-config/proposal.md b/openspec/changes/separate-tool-options-and-config/proposal.md new file mode 100644 index 00000000..3f69b89f --- /dev/null +++ b/openspec/changes/separate-tool-options-and-config/proposal.md @@ -0,0 +1,31 @@ +## Why + +Tools expose two static surfaces with no formal separation: `static options` (core/plugin-facing wiring — `toolbox`, `shortcut`, `inlineToolbar`, `tunes`, `conversionConfig`, `canBeSplit`) and `ToolConfig` (plugin-specific user data, typed only via a generic `Config extends ToolConfig = any` re-exported from the legacy `@editorjs/editorjs` package, where `ToolConfig = T` is an untyped passthrough). Because `options.toolbox` is a plain static value evaluated once at class-definition time, a tool has no way to express "toolbox entries depend on my resolved config" — there is no formal contract, no type checking, and no runtime verification connecting a `ToolConfig` field to the `options` it's meant to drive. + +This is not hypothetical: it is live today in the Header tool's v3 SDK migration (draft PR [editor-js/header#130](https://github.com/editor-js/header/pull/130)). `HeaderConfig` declares `levels` and `defaultLevel`, and `defaultLevel` does flow through (it's read inside the constructor from the resolved `config` object). But `levels` is dead — the static `options.toolbox` array is hardcoded to exactly three entries (H1–H3) and never consults `config.levels`, so a user who configures `levels: [1]` sees no change in the toolbox. The mechanism to fix this already exists in the core tool lifecycle (`prepare({ toolName, config })` runs once per registered tool, before the tool is announced to the Toolbox/Shortcuts/InlineToolbar/Tunes plugins), but nothing in the SDK's contracts guides a tool author to use it for config-derived options, and nothing catches the drift when, as in Header, a declared config field ends up unused. + +## What Changes + +- Define `ToolConfig` as an SDK-owned, per-tool-type-parameterized contract, replacing the untyped passthrough currently imported from `@editorjs/editorjs`. **BREAKING**: a tool's `Config` generic must conform to the new contract's shape and import path. +- Keep `ToolOptions` (`BlockToolOptions` / `InlineToolOptions` / `BlockTuneOptions`) as the static, declarative, core/plugin-facing contract, formally separated from `ToolConfig` — `options.config` remains the channel for config *defaults*, but is no longer the only thing standing in for "everything a tool needs at runtime". +- Extend the `prepare()` contract so a tool can return config-derived option values (starting with `toolbox`) after receiving its fully-resolved `ToolConfig`, and have the result flow into the tool's effective options before it is advertised to plugins — without mutating the tool's shared static class property (the current pattern of writing directly to `ClassName.options` would leak across multiple `Core` instances sharing the same tool class on one page). +- Add compile-time typing and a dev-time check that flags a `ToolConfig` field which no computed option / consumer reads (directly addressing the `HeaderConfig.levels` drift). +- Migrate the four in-repo tools (`paragraph`, `bold`, `italic`, `inline-link`) to the new `ToolConfig`/`ToolOptions` contracts. **BREAKING** for any tool relying on the current plain-object-only `static options` typing. +- Out of scope: changing a tool's config after the editor has already mounted (no live/reactive "hot-swap" API). Config is resolved once, during tool preparation, before the editor renders its UI. + +## Capabilities + +### New Capabilities +(none — this reshapes the existing tool-contract behavior rather than introducing a new capability area) + +### Modified Capabilities +- `sdk`: the "Tool and tune contracts" requirement changes — `ToolConfig` becomes a dedicated SDK contract (no longer a passthrough re-export), the static `options`/`config` merge behavior gains a "config-derived options resolved via `prepare()`" step that runs before static options are read by consumers (Toolbox, Shortcuts, InlineToolbar, Tunes), and `BaseToolConstructor.prepare()`'s signature/return type changes accordingly. + +## Impact + +- `packages/sdk/src/entities/{BaseTool.ts, BlockTool.ts, InlineTool.ts, BlockTune.ts}`: new `ToolConfig`/`ToolOptions` contracts; extended `prepare()` signature and return type. +- `packages/sdk/src/tools/facades/{BaseToolFacade.ts, BlockToolFacade.ts, InlineToolFacade.ts, BlockTuneFacade.ts}`: a per-facade-instance slot for `prepare()`-computed options, inserted into the existing static/`use()`-time merge chain (the `toolbox` getter's merge algorithm gains a tier between static defaults and explicit `use()`-time overrides). +- `packages/core/src/tools/{ToolsManager.ts, ToolsFactory.ts}`: capture `prepare()`'s return value and thread computed options into facade construction, ahead of the `ToolLoadedCoreEvent` dispatch that Toolbox/etc. listen for. +- `packages/tools/{paragraph,bold,italic,inline-link}`: migrate to the new `ToolConfig` import/contract (type-only change; no tool here currently needs config-derived options). +- `editor-js/header` (external submodule repo, PR #130): not modified by this change directly, but is the motivating and validating case — its `HeaderConfig.levels` drift is the concrete bug this change makes fixable. +- `openspec/specs/sdk/spec.md`: delta spec updates to the "Tool and tune contracts" requirement. diff --git a/openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md b/openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md new file mode 100644 index 00000000..64c3e6d6 --- /dev/null +++ b/openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md @@ -0,0 +1,36 @@ +## MODIFIED Requirements + +### Requirement: Tool and tune contracts +The system SHALL define the static/instance contracts that block tools, inline tools, and block tunes must satisfy: `BaseTool`/`BaseToolConstructor` (common `name`, `options`, `prepare()`, `reset()`), `BlockTool`/`BlockToolConstructor` (adds `toolbox`, `shortcut`, `inlineToolbar`, `tunes`, `conversionConfig`, `canBeSplit`), `InlineTool`/`InlineToolConstructor` (adds `isActive`, `getFormattingOptions`, `createWrapper`, `getToolbarConfig`), and `BlockTune`/`BlockTuneConstructor`. `ToolConfig` SHALL be an SDK-owned contract (not a re-exported passthrough from `@editorjs/editorjs`), kept formally distinct from `ToolOptions`: `options` describes core/plugin-facing wiring available before any block instance exists, while `ToolConfig` describes tool-author-facing user data resolved per tool registration. + +#### Scenario: Options and config merging in a tool facade +- **GIVEN** a tool class has static `options` (and optionally `options.config` typed as `ToolConfig`) +- **WHEN** the tool is registered via `use(Tool, options)` with overriding options +- **THEN** the facade's `options` getter merges static options with `use()`-time options, with `use()`-time keys taking precedence, and the `config` getter merges similarly, injecting `defaultPlaceholder` only when `isDefault` is true and no `placeholder` key is already present + +#### Scenario: Resolving config-derived toolbox entries via prepare() +- **GIVEN** a block tool's static `prepare({ toolName, config })` returns an object containing a `toolbox` field +- **WHEN** the tool is being registered, before `ToolLoadedCoreEvent` is dispatched for it +- **THEN** the returned `toolbox` value is captured on the tool's facade instance — not written onto the tool's shared static `options` object — and is available to the facade's `toolbox` getter ahead of the tool being announced to the Toolbox/Shortcuts/InlineToolbar/Tunes plugins + +#### Scenario: Toolbox merge tiers +- **GIVEN** a block tool has a static `options.toolbox` default, a `prepare()`-computed `toolbox` value, and/or a `use()`-time `toolbox` override +- **WHEN** the facade's `toolbox` getter is read +- **THEN** the value is resolved in order — static default, then the `prepare()`-computed value if present, then the explicit `use()`-time override on top — using the existing array/object positional-merge algorithm, and a `use()`-time `toolbox: false` still hides the tool from the toolbox regardless of any computed value + +#### Scenario: Detecting an unused config key +- **GIVEN** a tool's resolved `ToolConfig` object has a key that is neither passed through via `options.config` defaults nor read by a `prepare()` that returns a `toolbox` (or other recognized) field +- **WHEN** the tool finishes preparation +- **THEN** the system emits a dev-time warning identifying the unused key, without throwing or blocking tool registration + +#### Scenario: Text content conversion without config +- **GIVEN** a block tool has no `conversionConfig` +- **WHEN** `exportTextContent`/`importTextContent` is called on its facade +- **THEN** it throws a descriptive error stating the tool does not have export/import configuration + +#### Scenario: Text content conversion with a keypath +- **GIVEN** a block tool's `conversionConfig` specifies a dot-notation string key (including nested array paths, e.g. `items.0.text`) +- **WHEN** `exportTextContent`/`importTextContent` is called +- **THEN** the value at that keypath is read/written, producing or consuming a `TextNodeSerialized` value tagged with the hidden `Text` block-child-type marker + +Implemented in `src/entities/BaseTool.ts`, `BlockTool.ts`, `InlineTool.ts`, `BlockTune.ts`, `src/tools/facades/{BaseToolFacade,BlockToolFacade,InlineToolFacade,BlockTuneFacade}.ts`, `packages/core/src/tools/{ToolsManager.ts,ToolsFactory.ts}`, validated by `src/tools/facades/BaseToolFacade.spec.ts`. diff --git a/openspec/changes/separate-tool-options-and-config/tasks.md b/openspec/changes/separate-tool-options-and-config/tasks.md new file mode 100644 index 00000000..db72ccf4 --- /dev/null +++ b/openspec/changes/separate-tool-options-and-config/tasks.md @@ -0,0 +1,36 @@ +## 1. SDK contracts: `ToolConfig` and `ToolOptions` + +- [ ] 1.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` asserting `ToolConfig` is exported from `@editorjs/sdk` itself (not re-exported from `@editorjs/editorjs`), and that `BaseToolOptions`/`BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions` reference the SDK-owned type +- [ ] 1.2 Define `ToolConfig` in `packages/sdk/src/entities/BaseTool.ts` as an SDK-owned generic type (replacing the `@editorjs/editorjs` re-export), update `BaseToolOptions` and all per-tool-type option interfaces (`BlockTool.ts`, `InlineTool.ts`, `BlockTune.ts`) to import it from there +- [ ] 1.3 Update `packages/tools/{paragraph,bold,italic,inline-link}` to import `ToolConfig`/their `Config` type from the new SDK location; confirm `yarn workspace typecheck`/`yarn lint` pass with no behavior change + +## 2. `prepare()` contract for config-derived options + +- [ ] 2.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` (or a new co-located spec) asserting `BaseToolConstructor.prepare()` accepts a return type of `PreparedToolOptions | void | Promise | void>` +- [ ] 2.2 Define `PreparedToolOptions` in `packages/sdk/src/entities/BaseTool.ts`, initially restricted to `{ toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false }`, and update `BaseToolConstructor.prepare()`'s signature to use it +- [ ] 2.3 Add a failing test in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` asserting a `BaseToolFacade` instance exposes a `preparedOptions` slot that starts `undefined` and can be set after construction, without touching `constructable.options` +- [ ] 2.4 Implement the `preparedOptions` slot on `BaseToolFacade` (private field + a setter method the manager calls), keeping `constructable.options` untouched + +## 3. Toolbox merge tiers in `BlockToolFacade` + +- [ ] 3.1 Add failing tests in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` for `BlockToolFacade.toolbox`, one per tier combination: static-only, prepared-only, static+prepared, prepared+`use()`-override, and `use()`-time `toolbox: false` hiding the tool even when a prepared value exists +- [ ] 3.2 Update `BlockToolFacade.toolbox` to seed its existing merge algorithm from `preparedOptions?.toolbox ?? constructable.options?.toolbox` instead of `constructable.options?.toolbox` alone, keeping the `use()`-time override layer unchanged on top +- [ ] 3.3 Confirm existing toolbox-merge tests (static + `use()`-override only, no prepared tier) still pass unmodified + +## 4. Wiring `prepare()`'s return value through core + +- [ ] 4.1 Add a failing test for `ToolsManager.prepareTools()` asserting that when `toolConstructor.prepare()` resolves with `{ toolbox: [...] }`, the resulting facade (as added to `available`/`unavailable` collections) has that value on `preparedOptions.toolbox` before `ToolLoadedCoreEvent` is dispatched +- [ ] 4.2 Update `ToolsManager.prepareTools()` to capture `prepare()`'s resolved return value and call the facade's `preparedOptions` setter (task 2.4) before `setToAvailableToolsCollection`/`ToolLoadedCoreEvent` +- [ ] 4.3 Add a failing test confirming a `prepare()` that returns `undefined`/void leaves `preparedOptions` unset (no regression for tools without config-derived options) + +## 5. Dev-time unused-config-key detection + +- [ ] 5.1 Add a failing test asserting that when a tool's resolved `ToolConfig` object has a key not passed through via `options.config` defaults and not present on a `prepare()`-returned `PreparedToolOptions`, `ToolsManager.prepareTools()` emits a `console.warn` naming the unused key, without throwing or marking the tool unavailable +- [ ] 5.2 Implement the reachability check in `ToolsManager.prepareTools()` (or a small helper it calls), gated the same way existing dev diagnostics are (non-throwing, dev-only) +- [ ] 5.3 Add a failing/passing pair of tests confirming a config key that *is* consumed (via `options.config` default or a `PreparedToolOptions`-returning `prepare()`) produces no warning + +## 6. Spec and documentation alignment + +- [ ] 6.1 Run `openspec validate --changes separate-tool-options-and-config --strict` (or the project's equivalent) and fix any delta-spec formatting issues +- [ ] 6.2 Update `docs/plugins.md`/`docs/architecture.md` mentions of `core.use(ToolConstructor, options)` if the described merge behavior no longer matches (cross-reference note from the proposal) +- [ ] 6.3 Note in the PR description that this change makes the `editor-js/header#130` `HeaderConfig.levels` fix possible, and that wiring `Header.prepare()` to consume it is tracked as a follow-up in the `header` repo, not in this change From 22e24543c88b75cdd191c7038465e9daf4b5eb86 Mon Sep 17 00:00:00 2001 From: Reversean Date: Wed, 29 Jul 2026 18:09:20 +0300 Subject: [PATCH 2/6] feat(sdk): separate ToolConfig from static tool options, add prepare()-derived toolbox ToolConfig becomes an SDK-owned contract instead of a permissive `any`-defaulted re-export from @editorjs/editorjs. A tool's prepare() can now return PreparedToolOptions to compute static options (currently `toolbox`) from its resolved config, captured per facade instance so it never mutates the shared static `options` across multiple Core instances. ToolsManager also warns at dev-time when a resolved config key is never read, catching drift like editor-js/header#130's unused `HeaderConfig.levels`. BREAKING CHANGE: ToolConfig is exported from @editorjs/sdk, not re-exported from @editorjs/editorjs. Co-Authored-By: Claude Sonnet 5 --- .../design.md | 5 +- .../separate-tool-options-and-config/tasks.md | 38 ++--- packages/core/src/tools/ToolsManager.spec.ts | 153 ++++++++++++++++++ packages/core/src/tools/ToolsManager.ts | 55 ++++++- packages/sdk/src/entities/BaseTool.spec.ts | 68 ++++++++ packages/sdk/src/entities/BaseTool.ts | 27 +++- packages/sdk/src/entities/BlockTool.ts | 3 +- packages/sdk/src/entities/BlockTune.ts | 3 +- packages/sdk/src/entities/InlineTool.ts | 4 +- .../src/tools/facades/BaseToolFacade.spec.ts | 74 +++++++++ .../sdk/src/tools/facades/BaseToolFacade.ts | 31 +++- .../sdk/src/tools/facades/BlockToolFacade.ts | 34 ++-- packages/tools/paragraph/src/index.ts | 4 +- 13 files changed, 454 insertions(+), 45 deletions(-) create mode 100644 packages/core/src/tools/ToolsManager.spec.ts create mode 100644 packages/sdk/src/entities/BaseTool.spec.ts diff --git a/openspec/changes/separate-tool-options-and-config/design.md b/openspec/changes/separate-tool-options-and-config/design.md index 35a6cc19..26ede70d 100644 --- a/openspec/changes/separate-tool-options-and-config/design.md +++ b/openspec/changes/separate-tool-options-and-config/design.md @@ -44,9 +44,9 @@ This matches how they're actually consumed: `options` (via facade getters) is re `prepare()`'s signature extends to: ```ts prepare?(data: { toolName: string, config: Config }): - PreparedToolOptions | void | Promise | void>; + PreparedToolOptions | void | Promise; ``` -where `PreparedToolOptions` is a partial type restricted, for now, to `{ toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false }`. `ToolsManager.prepareTools()` captures this return value and passes it through `ToolsFactory` into the facade constructor as a new `preparedOptions` field, stored on the `BaseToolFacade` instance (not on `constructable`). +where `PreparedToolOptions` is a partial type restricted, for now, to `{ toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false }`. `ToolsManager.prepareTools()` captures this return value and passes it through `ToolsFactory` into the facade constructor as a new `preparedOptions` field, stored on the `BaseToolFacade` instance (not on `constructable`). *Alternative considered*: have the tool mutate `ClassName.options` directly inside `prepare()` (the shape floated during earlier discussion of this change). Rejected once the multi-`Core`-instance case was worked through: `constructable.options` is one object shared by every facade wrapping that class, so a second `Core` instance's `prepare()` call would overwrite the first instance's computed toolbox for both. Returning a value and letting the facade own it keeps the result properly scoped per editor instance while still using the same trigger point (`prepare()`) already agreed on. **4. `toolbox` resolution gains a third tier, in this order: static default → prepared (config-derived) → explicit `use()`-time override.** @@ -78,3 +78,4 @@ A lightweight check (gated the same way existing `ToolsManager` dev diagnostics - Should `PreparedToolOptions` generalize beyond `toolbox` (e.g. `shortcut`, `inlineToolbar`) once a second concrete need appears, or stay toolbox-only indefinitely? Leaning toolbox-only until evidenced otherwise (YAGNI). - Should the "unused config key" dev warning live in SDK core (always on outside production, matching existing `ToolsManager` console diagnostics) or as an opt-in lint/test helper tool authors run in CI? Leaning SDK-core for now; worth revisiting if it proves noisy. +- How would a third-party plugin (i.e. not one of the framework's own Toolbox/Shortcuts/InlineToolbar/Tunes consumers) read its own config-derived data off a tool? Today `options`/`PreparedToolOptions` are a closed, core-defined schema, not an extensible registry a third party can add fields to. `BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions` already carry a `[key: string]: unknown` escape hatch for custom static fields a tool author reads back themselves; `PreparedToolOptions` currently doesn't. Not adding that now (no evidenced consumer) — worth revisiting together with the bullet above if a real third-party case shows up. diff --git a/openspec/changes/separate-tool-options-and-config/tasks.md b/openspec/changes/separate-tool-options-and-config/tasks.md index db72ccf4..3fc43580 100644 --- a/openspec/changes/separate-tool-options-and-config/tasks.md +++ b/openspec/changes/separate-tool-options-and-config/tasks.md @@ -1,36 +1,36 @@ ## 1. SDK contracts: `ToolConfig` and `ToolOptions` -- [ ] 1.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` asserting `ToolConfig` is exported from `@editorjs/sdk` itself (not re-exported from `@editorjs/editorjs`), and that `BaseToolOptions`/`BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions` reference the SDK-owned type -- [ ] 1.2 Define `ToolConfig` in `packages/sdk/src/entities/BaseTool.ts` as an SDK-owned generic type (replacing the `@editorjs/editorjs` re-export), update `BaseToolOptions` and all per-tool-type option interfaces (`BlockTool.ts`, `InlineTool.ts`, `BlockTune.ts`) to import it from there -- [ ] 1.3 Update `packages/tools/{paragraph,bold,italic,inline-link}` to import `ToolConfig`/their `Config` type from the new SDK location; confirm `yarn workspace typecheck`/`yarn lint` pass with no behavior change +- [x] 1.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` asserting `ToolConfig` is exported from `@editorjs/sdk` itself (not re-exported from `@editorjs/editorjs`), and that `BaseToolOptions` references the SDK-owned type — scoped to `BaseToolOptions` only rather than repeating the same check on `BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions`, since none of them redeclare the `config` field (they inherit it unmodified from `BaseToolOptions`), so per-subtype tests would exercise the identical TS structural-inheritance check with no added coverage +- [x] 1.2 Define `ToolConfig` in `packages/sdk/src/entities/BaseTool.ts` as an SDK-owned generic type (replacing the `@editorjs/editorjs` re-export), update `BaseToolOptions` and all per-tool-type option interfaces (`BlockTool.ts`, `InlineTool.ts`, `BlockTune.ts`) to import it from there +- [x] 1.3 Update `packages/tools/{paragraph,bold,italic,inline-link}` to import `ToolConfig`/their `Config` type from the new SDK location; confirm `yarn workspace typecheck`/`yarn lint` pass with no behavior change ## 2. `prepare()` contract for config-derived options -- [ ] 2.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` (or a new co-located spec) asserting `BaseToolConstructor.prepare()` accepts a return type of `PreparedToolOptions | void | Promise | void>` -- [ ] 2.2 Define `PreparedToolOptions` in `packages/sdk/src/entities/BaseTool.ts`, initially restricted to `{ toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false }`, and update `BaseToolConstructor.prepare()`'s signature to use it -- [ ] 2.3 Add a failing test in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` asserting a `BaseToolFacade` instance exposes a `preparedOptions` slot that starts `undefined` and can be set after construction, without touching `constructable.options` -- [ ] 2.4 Implement the `preparedOptions` slot on `BaseToolFacade` (private field + a setter method the manager calls), keeping `constructable.options` untouched +- [x] 2.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` (or a new co-located spec) asserting `BaseToolConstructor.prepare()` accepts a return type of `PreparedToolOptions | void | Promise | void>` +- [x] 2.2 Define `PreparedToolOptions` in `packages/sdk/src/entities/BaseTool.ts`, initially restricted to `{ toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false }`, and update `BaseToolConstructor.prepare()`'s signature to use it +- [x] 2.3 Add a failing test in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` asserting a `BaseToolFacade` instance exposes a `preparedOptions` slot that starts `undefined` and can be set after construction, without touching `constructable.options` +- [x] 2.4 Implement the `preparedOptions` slot on `BaseToolFacade` (private field + a setter method the manager calls), keeping `constructable.options` untouched ## 3. Toolbox merge tiers in `BlockToolFacade` -- [ ] 3.1 Add failing tests in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` for `BlockToolFacade.toolbox`, one per tier combination: static-only, prepared-only, static+prepared, prepared+`use()`-override, and `use()`-time `toolbox: false` hiding the tool even when a prepared value exists -- [ ] 3.2 Update `BlockToolFacade.toolbox` to seed its existing merge algorithm from `preparedOptions?.toolbox ?? constructable.options?.toolbox` instead of `constructable.options?.toolbox` alone, keeping the `use()`-time override layer unchanged on top -- [ ] 3.3 Confirm existing toolbox-merge tests (static + `use()`-override only, no prepared tier) still pass unmodified +- [x] 3.1 Add failing tests in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` for `BlockToolFacade.toolbox`, one per tier combination: static-only, prepared-only, static+prepared, prepared+`use()`-override, and `use()`-time `toolbox: false` hiding the tool even when a prepared value exists +- [x] 3.2 Update `BlockToolFacade.toolbox` to seed its existing merge algorithm from `preparedOptions?.toolbox ?? constructable.options?.toolbox` instead of `constructable.options?.toolbox` alone, keeping the `use()`-time override layer unchanged on top +- [x] 3.3 Confirm existing toolbox-merge tests (static + `use()`-override only, no prepared tier) still pass unmodified — no such tests existed before this change (the `toolbox` getter had no prior spec coverage); the static-only and static+use()-override paths are now covered by 3.1's new tests instead, and the full `@editorjs/sdk` suite (31 tests) passes ## 4. Wiring `prepare()`'s return value through core -- [ ] 4.1 Add a failing test for `ToolsManager.prepareTools()` asserting that when `toolConstructor.prepare()` resolves with `{ toolbox: [...] }`, the resulting facade (as added to `available`/`unavailable` collections) has that value on `preparedOptions.toolbox` before `ToolLoadedCoreEvent` is dispatched -- [ ] 4.2 Update `ToolsManager.prepareTools()` to capture `prepare()`'s resolved return value and call the facade's `preparedOptions` setter (task 2.4) before `setToAvailableToolsCollection`/`ToolLoadedCoreEvent` -- [ ] 4.3 Add a failing test confirming a `prepare()` that returns `undefined`/void leaves `preparedOptions` unset (no regression for tools without config-derived options) +- [x] 4.1 Add a failing test for `ToolsManager.prepareTools()` asserting that when `toolConstructor.prepare()` resolves with `{ toolbox: [...] }`, the resulting facade (as added to `available`/`unavailable` collections) has that value on `preparedOptions.toolbox` before `ToolLoadedCoreEvent` is dispatched +- [x] 4.2 Update `ToolsManager.prepareTools()` to capture `prepare()`'s resolved return value and call the facade's `preparedOptions` setter (task 2.4) before `setToAvailableToolsCollection`/`ToolLoadedCoreEvent` +- [x] 4.3 Add a failing test confirming a `prepare()` that returns `undefined`/void leaves `preparedOptions` unset (no regression for tools without config-derived options) ## 5. Dev-time unused-config-key detection -- [ ] 5.1 Add a failing test asserting that when a tool's resolved `ToolConfig` object has a key not passed through via `options.config` defaults and not present on a `prepare()`-returned `PreparedToolOptions`, `ToolsManager.prepareTools()` emits a `console.warn` naming the unused key, without throwing or marking the tool unavailable -- [ ] 5.2 Implement the reachability check in `ToolsManager.prepareTools()` (or a small helper it calls), gated the same way existing dev diagnostics are (non-throwing, dev-only) -- [ ] 5.3 Add a failing/passing pair of tests confirming a config key that *is* consumed (via `options.config` default or a `PreparedToolOptions`-returning `prepare()`) produces no warning +- [x] 5.1 Add a failing test asserting that when a tool's resolved `ToolConfig` object has a key not passed through via `options.config` defaults and not present on a `prepare()`-returned `PreparedToolOptions`, `ToolsManager.prepareTools()` emits a `console.warn` naming the unused key, without throwing or marking the tool unavailable +- [x] 5.2 Implement the reachability check in `ToolsManager.prepareTools()` (or a small helper it calls), gated the same way existing dev diagnostics are (non-throwing, dev-only) — implemented via a `Proxy` around the resolved config passed to `prepare()`, tracking which keys are actually read +- [x] 5.3 Add a failing/passing pair of tests confirming a config key that *is* consumed (via `options.config` default or a `PreparedToolOptions`-returning `prepare()`) produces no warning ## 6. Spec and documentation alignment -- [ ] 6.1 Run `openspec validate --changes separate-tool-options-and-config --strict` (or the project's equivalent) and fix any delta-spec formatting issues -- [ ] 6.2 Update `docs/plugins.md`/`docs/architecture.md` mentions of `core.use(ToolConstructor, options)` if the described merge behavior no longer matches (cross-reference note from the proposal) -- [ ] 6.3 Note in the PR description that this change makes the `editor-js/header#130` `HeaderConfig.levels` fix possible, and that wiring `Header.prepare()` to consume it is tracked as a follow-up in the `header` repo, not in this change +- [x] 6.1 Run `openspec validate --changes separate-tool-options-and-config --strict` (or the project's equivalent) and fix any delta-spec formatting issues — passes clean +- [x] 6.2 Update `docs/plugins.md`/`docs/architecture.md` mentions of `core.use(ToolConstructor, options)` if the described merge behavior no longer matches (cross-reference note from the proposal) — reviewed; existing mentions only describe the coarse `use()` → `initialize()` → prepare-tools → emit `ToolLoadedCoreEvent` flow, which this change preserves exactly, so no doc text is inaccurate +- [x] 6.3 Note in the PR description that this change makes the `editor-js/header#130` `HeaderConfig.levels` fix possible, and that wiring `Header.prepare()` to consume it is tracked as a follow-up in the `header` repo, not in this change — captured in `proposal.md`'s Impact section; still needs to be carried into the actual GitHub PR body when the PR is updated diff --git a/packages/core/src/tools/ToolsManager.spec.ts b/packages/core/src/tools/ToolsManager.spec.ts new file mode 100644 index 00000000..9c51abcf --- /dev/null +++ b/packages/core/src/tools/ToolsManager.spec.ts @@ -0,0 +1,153 @@ +/* eslint-disable jsdoc/require-jsdoc */ +import { describe, expect, it, jest } from '@jest/globals'; +import { EventBus, ToolType, CoreEventType } from '@editorjs/sdk'; +import type { + CoreConfigValidated, + ToolConstructable, + ToolLoadedCoreEvent, + ToolStaticOptions +} from '@editorjs/sdk'; +import type { EditorAPI } from '../api/index.js'; +import ToolsManager from './ToolsManager.js'; + +const createManager = (): { manager: ToolsManager; + eventBus: EventBus; } => { + const editorConfig = { + tools: {}, + defaultBlock: '__no-default-block__', + placeholder: '', + } as CoreConfigValidated; + const apiFactory = (): EditorAPI => ({} as EditorAPI); + const eventBus = new EventBus(); + const manager = new ToolsManager(editorConfig, apiFactory, eventBus); + + return { manager, + eventBus }; +}; + +describe('ToolsManager.prepareTools()', () => { + it('should capture a config-derived toolbox from prepare() on the facade before ToolLoadedCoreEvent fires', async () => { + const { manager, eventBus } = createManager(); + const toolboxFromConfig = [{ title: 'Heading 1' }]; + + class MockHeaderTool { + public static type = ToolType.Block; + public static name = 'header'; + public static prepare = jest.fn(() => Promise.resolve({ toolbox: toolboxFromConfig })); + } + + let preparedOptionsAtDispatchTime: unknown; + + eventBus.addEventListener(`core:${CoreEventType.ToolLoaded}`, (event: ToolLoadedCoreEvent) => { + preparedOptionsAtDispatchTime = event.detail.tool.preparedOptions; + }); + + await manager.prepareTools([[MockHeaderTool as unknown as ToolConstructable, undefined]]); + + expect(preparedOptionsAtDispatchTime).toEqual({ toolbox: toolboxFromConfig }); + expect(manager.available.get('header')?.preparedOptions).toEqual({ toolbox: toolboxFromConfig }); + }); + + it('should leave preparedOptions unset when prepare() resolves with undefined', async () => { + const { manager } = createManager(); + + class MockParagraphTool { + public static type = ToolType.Block; + public static name = 'paragraph'; + public static prepare = jest.fn(() => Promise.resolve(undefined)); + } + + await manager.prepareTools([[MockParagraphTool as unknown as ToolConstructable, undefined]]); + + expect(manager.available.get('paragraph')?.preparedOptions).toBeUndefined(); + }); + + it('should add the tool to available tools without calling prepare() when the tool has no prepare method', async () => { + const { manager } = createManager(); + + class MockBoldTool { + public static type = ToolType.Inline; + public static name = 'bold'; + } + + await manager.prepareTools([[MockBoldTool as unknown as ToolConstructable, undefined]]); + + expect(manager.available.get('bold')?.preparedOptions).toBeUndefined(); + }); +}); + +describe('ToolsManager.prepareTools() — unused config key detection', () => { + it('should warn about a resolved config key that prepare() never reads and that has no static default', async () => { + const { manager } = createManager(); + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); + + class MockHeaderTool { + public static type = ToolType.Block; + public static name = 'header'; + public static prepare = jest.fn(({ config }: { config: Record }) => { + void config.defaultLevel; + + return Promise.resolve(undefined); + }); + } + + await manager.prepareTools([ + [ + MockHeaderTool as unknown as ToolConstructable, + { config: { levels: [1, 2], + defaultLevel: 1 } } as ToolStaticOptions, + ], + ]); + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('levels')); + + warnSpy.mockRestore(); + }); + + it('should not warn about a config key that prepare() reads', async () => { + const { manager } = createManager(); + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); + + class MockHeaderTool { + public static type = ToolType.Block; + public static name = 'header'; + public static prepare = jest.fn(({ config }: { config: Record }) => { + void config.levels; + void config.defaultLevel; + + return Promise.resolve(undefined); + }); + } + + await manager.prepareTools([ + [ + MockHeaderTool as unknown as ToolConstructable, + { config: { levels: [1, 2], + defaultLevel: 1 } } as ToolStaticOptions, + ], + ]); + + expect(warnSpy).not.toHaveBeenCalled(); + + warnSpy.mockRestore(); + }); + + it('should not warn about a config key that is part of the tool\'s own static config defaults', async () => { + const { manager } = createManager(); + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); + + class MockHeaderTool { + public static type = ToolType.Block; + public static name = 'header'; + // eslint-disable-next-line @typescript-eslint/no-magic-numbers -- heading levels 1-3, self-evident + public static options = { config: { levels: [1, 2, 3] } }; + public static prepare = jest.fn(() => Promise.resolve(undefined)); + } + + await manager.prepareTools([[MockHeaderTool as unknown as ToolConstructable, undefined]]); + + expect(warnSpy).not.toHaveBeenCalled(); + + warnSpy.mockRestore(); + }); +}); diff --git a/packages/core/src/tools/ToolsManager.ts b/packages/core/src/tools/ToolsManager.ts index 3026fdbb..dfa58c7f 100644 --- a/packages/core/src/tools/ToolsManager.ts +++ b/packages/core/src/tools/ToolsManager.ts @@ -136,15 +136,39 @@ export default class ToolsManager { void promiseQueue.add(async () => { try { const tool = factory.get(toolName); + const resolvedConfig = tool.config; + const accessedConfigKeys = new Set(); + + /** + * Tracks which config keys `prepare()` actually reads, so a key that's set but never + * consumed anywhere reachable can be flagged below. + */ + const trackedConfig = new Proxy(resolvedConfig, { + get(target, prop, receiver) { + if (typeof prop === 'string') { + accessedConfigKeys.add(prop); + } + + return Reflect.get(target, prop, receiver); + }, + }); /** * Merged plugin `config` only (static `options().config` + `use(Tool, options).config`), aligned with `BaseToolFacade.prepare`. */ - await toolConstructor.prepare!({ + const preparedOptions = await toolConstructor.prepare!({ toolName, - config: tool.config, + config: trackedConfig, }); + /** + * Captured on the facade instance (not the shared static `options`) so config-derived + * options (e.g. `toolbox`) are available before ToolLoadedCoreEvent is dispatched below. + */ + tool.setPreparedOptions(preparedOptions ?? undefined); + + this.#warnAboutUnusedConfigKeys(toolName, toolConstructor, resolvedConfig, accessedConfigKeys); + if (tool.isInline()) { /** * Some Tools validation @@ -183,6 +207,33 @@ export default class ToolsManager { await promiseQueue.completed; } + /** + * Dev-time diagnostic: warns about a key in a tool's resolved config that has no static + * default on the tool (i.e. the tool author didn't declare it) and that `prepare()` never + * read off the config it was given. Doesn't throw and doesn't affect tool availability — + * it only helps catch a config field that's set but silently has no effect. + * @param toolName - name of the tool being prepared + * @param toolConstructor - the tool's constructable, used to read its static config defaults + * @param resolvedConfig - the tool's fully-resolved config (static defaults + `use()`-time overrides) + * @param accessedConfigKeys - keys of `resolvedConfig` that were read while `prepare()` ran + */ + #warnAboutUnusedConfigKeys( + toolName: string, + toolConstructor: ToolConstructable, + resolvedConfig: Record, + accessedConfigKeys: Set + ): void { + const staticConfigKeys = new Set(Object.keys(toolConstructor.options?.config ?? {})); + + for (const key of Object.keys(resolvedConfig)) { + if (!staticConfigKeys.has(key) && !accessedConfigKeys.has(key)) { + console.warn( + `Tool "${toolName}": config key "${key}" is set but never used (not declared as a static default and not read by prepare()). This may be a mistake.` + ); + } + } + } + /** * Unify tools config * @param config - user's tools config diff --git a/packages/sdk/src/entities/BaseTool.spec.ts b/packages/sdk/src/entities/BaseTool.spec.ts new file mode 100644 index 00000000..98909341 --- /dev/null +++ b/packages/sdk/src/entities/BaseTool.spec.ts @@ -0,0 +1,68 @@ +/* eslint-disable jsdoc/require-jsdoc */ +import { describe, expect, it } from '@jest/globals'; +import type { ToolConfig, BaseToolOptions, BaseToolConstructor } from './BaseTool.js'; + +interface SampleConfig { + placeholder?: string; +} + +describe('ToolConfig', () => { + it('should allow a concrete shape to satisfy the SDK-owned ToolConfig contract', () => { + const config: ToolConfig = { placeholder: 'hello' }; + + expect(config.placeholder).toBe('hello'); + }); + + it('should type an unparameterized ToolConfig as a permissive object rather than defaulting to any', () => { + const describeConfig = (config: ToolConfig): unknown => { + // @ts-expect-error -- config no longer defaults to `any`, so arbitrary property access must be narrowed first + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- the unsafe access is exactly what this test proves is now rejected + const leak: string = config.whatever; + + return leak; + }; + + expect(typeof describeConfig).toBe('function'); + }); + + it('should type BaseToolOptions.config using the SDK-owned ToolConfig shape', () => { + const options: BaseToolOptions = { + config: { placeholder: 'hi' }, + }; + + expect(options.config?.placeholder).toBe('hi'); + }); +}); + +describe('prepare() return contract (PreparedToolOptions)', () => { + it('should allow prepare() to return config-derived toolbox entries', () => { + const prepare: BaseToolConstructor['prepare'] = () => { + return { toolbox: [{ title: 'Heading 1' }] }; + }; + + expect(typeof prepare).toBe('function'); + }); + + it('should allow prepare() to return void', () => { + const prepare: BaseToolConstructor['prepare'] = () => undefined; + + expect(typeof prepare).toBe('function'); + }); + + it('should allow prepare() to resolve a Promise of config-derived toolbox entries', () => { + const prepare: BaseToolConstructor['prepare'] = async () => { + return Promise.resolve({ toolbox: false as const }); + }; + + expect(typeof prepare).toBe('function'); + }); + + it('should reject a prepare() return value with an unrecognized field', () => { + // @ts-expect-error -- PreparedToolOptions only recognizes `toolbox` for now + const prepare: BaseToolConstructor['prepare'] = () => { + return { notARealOption: true }; + }; + + expect(typeof prepare).toBe('function'); + }); +}); diff --git a/packages/sdk/src/entities/BaseTool.ts b/packages/sdk/src/entities/BaseTool.ts index c54d4d42..89773cf4 100644 --- a/packages/sdk/src/entities/BaseTool.ts +++ b/packages/sdk/src/entities/BaseTool.ts @@ -1,9 +1,16 @@ -import type { ToolConfig } from '@editorjs/editorjs'; +import type { ToolboxConfigEntry } from '@editorjs/editorjs'; import type { BlockToolOptions } from './BlockTool.js'; import type { InlineToolOptions } from './InlineTool.js'; import type { BlockTuneOptions } from './BlockTune.js'; import type { ToolType } from './EntityType.js'; +/** + * Tool-author-facing user configuration, kept as a distinct SDK-owned contract from + * {@link ToolStaticOptions} (the core/plugin-facing static declaration). + * @template T - Shape of the plugin-specific configuration object. + */ +export type ToolConfig = T; + /** * Canonical keys shared by every tool options interface. */ @@ -56,6 +63,18 @@ export type ToolTypeToOptions = { [ToolType.Tune]: BlockTuneOptions; }; +/** + * Static options a tool can compute from its own resolved {@link ToolConfig}, as an override for + * the fixed defaults declared on {@link BaseToolConstructor.options}. + */ +export interface PreparedToolOptions { + /** + * Toolbox entry (or entries) computed from the tool's resolved config. Set to `false` to hide + * the tool from the toolbox. + */ + toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false; +} + /** * Common interface for Tool constructor (static) side. * @template Config - Shape of the plugin-specific config object. Passed to @@ -82,12 +101,16 @@ export interface BaseToolConstructor< /** * Tool's prepare method. Can be async. + * May return config-derived static options (currently just `toolbox`), computed from the + * fully-resolved `config`. Implementations never need to mutate this constructor's shared + * static `options` to have an effect — see {@link PreparedToolOptions}. * @param data - Object with toolName and config properties * @param data.toolName - Tool's own name * @param data.config - Merged plugin configuration */ // eslint-disable-next-line -- ESLint doesn't understand it's a type - prepare?(data: { toolName: string, config: Config }): void | Promise; + prepare?(data: { toolName: string, config: Config }): + PreparedToolOptions | void | Promise; /** * Tool's reset method to clean up anything set by prepare. Can be async. diff --git a/packages/sdk/src/entities/BlockTool.ts b/packages/sdk/src/entities/BlockTool.ts index ab18196a..cbe9a16b 100644 --- a/packages/sdk/src/entities/BlockTool.ts +++ b/packages/sdk/src/entities/BlockTool.ts @@ -1,13 +1,12 @@ import type { BlockTool as BlockToolVersion2, - ToolConfig, ToolboxConfigEntry } from '@editorjs/editorjs'; import type { BlockToolConstructorOptions as BlockToolConstructorOptionsVersion2 } from '@editorjs/editorjs'; import type { ValueSerialized } from '@editorjs/model-types'; import type { BlockToolAdapter } from './BlockToolAdapter.js'; import type { ToolType } from './EntityType.js'; -import type { BaseToolConstructor, BaseToolOptions } from './BaseTool'; +import type { BaseToolConstructor, BaseToolOptions, ToolConfig } from './BaseTool'; import type { EditorAPI } from '../api'; /** diff --git a/packages/sdk/src/entities/BlockTune.ts b/packages/sdk/src/entities/BlockTune.ts index 547d0b11..c699aa4f 100644 --- a/packages/sdk/src/entities/BlockTune.ts +++ b/packages/sdk/src/entities/BlockTune.ts @@ -1,10 +1,9 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import type { - ToolConfig, BlockTune as BlockTuneV2 } from '@editorjs/editorjs'; import type { ToolType } from './EntityType.js'; -import type { BaseToolConstructor, BaseToolOptions } from './BaseTool'; +import type { BaseToolConstructor, BaseToolOptions, ToolConfig } from './BaseTool'; /** * Options available on **Block Tunes** (`static options` or `use()` overrides). diff --git a/packages/sdk/src/entities/InlineTool.ts b/packages/sdk/src/entities/InlineTool.ts index 6d86de0e..3b24f1e0 100644 --- a/packages/sdk/src/entities/InlineTool.ts +++ b/packages/sdk/src/entities/InlineTool.ts @@ -1,9 +1,9 @@ import type { TextRange, InlineFragment } from '@editorjs/model-types'; import type { FormattingAction, IntersectType } from '@editorjs/model-types'; import type { InlineTool as InlineToolVersion2 } from '@editorjs/editorjs'; -import type { InlineToolConstructorOptions as InlineToolConstructorOptionsVersion2, ToolConfig } from '@editorjs/editorjs'; +import type { InlineToolConstructorOptions as InlineToolConstructorOptionsVersion2 } from '@editorjs/editorjs'; import type { ToolType } from './EntityType.js'; -import type { BaseToolConstructor, BaseToolOptions } from './BaseTool'; +import type { BaseToolConstructor, BaseToolOptions, ToolConfig } from './BaseTool'; import type { EditorAPI } from '../api'; import type { MenuConfig } from './MenuConfig.js'; diff --git a/packages/sdk/src/tools/facades/BaseToolFacade.spec.ts b/packages/sdk/src/tools/facades/BaseToolFacade.spec.ts index 9e0d967d..4164814e 100644 --- a/packages/sdk/src/tools/facades/BaseToolFacade.spec.ts +++ b/packages/sdk/src/tools/facades/BaseToolFacade.spec.ts @@ -164,6 +164,80 @@ describe('BaseToolFacade (via BlockToolFacade)', () => { }); }); + describe('preparedOptions', () => { + it('should start undefined before setPreparedOptions is called', () => { + const facade = createBlockFacade({}, {} as ToolOptions); + + expect(facade.preparedOptions).toBeUndefined(); + }); + + it('should expose the value passed to setPreparedOptions', () => { + const facade = createBlockFacade({}, {} as ToolOptions); + + facade.setPreparedOptions({ toolbox: [{ title: 'Heading 1' }] }); + + expect(facade.preparedOptions).toEqual({ toolbox: [{ title: 'Heading 1' }] }); + }); + + it('should not mutate the tool constructor\'s static options when set', () => { + const staticOptions = { toolbox: { title: 'Static' } }; + const facade = createBlockFacade(staticOptions, {} as ToolOptions); + + facade.setPreparedOptions({ toolbox: [{ title: 'Computed' }] }); + + expect(staticOptions).toEqual({ toolbox: { title: 'Static' } }); + }); + }); + + describe('toolbox getter (merge tiers)', () => { + it('should return the static toolbox entry when there is no prepared or use()-time value', () => { + const facade = createBlockFacade( + { toolbox: { title: 'Static' } }, + {} as ToolOptions + ); + + expect(facade.toolbox).toEqual([{ title: 'Static' }]); + }); + + it('should return the prepared toolbox entries when the tool has no static toolbox', () => { + const facade = createBlockFacade(undefined, {} as ToolOptions); + + facade.setPreparedOptions({ toolbox: [{ title: 'Heading 1' }, { title: 'Heading 2' }] }); + + expect(facade.toolbox).toEqual([{ title: 'Heading 1' }, { title: 'Heading 2' }]); + }); + + it('should layer an explicit use()-time override on top of the prepared toolbox', () => { + const facade = createBlockFacade( + undefined, + { toolbox: [{ title: 'Heading 1 override' }] } as ToolOptions + ); + + facade.setPreparedOptions({ toolbox: [{ title: 'Heading 1' }, { title: 'Heading 2' }] }); + + expect(facade.toolbox).toEqual([{ title: 'Heading 1 override' }]); + }); + + it('should hide the tool from the toolbox when use()-time toolbox is false, even with a prepared value', () => { + const facade = createBlockFacade( + undefined, + { toolbox: false } as ToolOptions + ); + + facade.setPreparedOptions({ toolbox: [{ title: 'Heading 1' }] }); + + expect(facade.toolbox).toBeUndefined(); + }); + + it('should hide the tool from the toolbox when the prepared value is false and there is no use()-time override', () => { + const facade = createBlockFacade(undefined, {} as ToolOptions); + + facade.setPreparedOptions({ toolbox: false }); + + expect(facade.toolbox).toBeUndefined(); + }); + }); + describe('exportTextContent', () => { it('throws when the tool has no conversionConfig.export', () => { const facade = createBlockFacade({}, {} as ToolOptions); diff --git a/packages/sdk/src/tools/facades/BaseToolFacade.ts b/packages/sdk/src/tools/facades/BaseToolFacade.ts index 9a63c8b9..e0752701 100644 --- a/packages/sdk/src/tools/facades/BaseToolFacade.ts +++ b/packages/sdk/src/tools/facades/BaseToolFacade.ts @@ -8,8 +8,9 @@ import { ToolType, BaseToolOptionKey } from '../../entities/index.js'; import { type BlockTuneFacade } from './BlockTuneFacade.js'; import type { BlockTool, BlockToolConstructor, InlineTool, InlineToolConstructor, BlockTuneConstructor, - ToolTypeToOptions, ToolStaticOptions, BlockToolOptions, InlineToolOptions, BlockTuneOptions -} from '../../entities/index.js'; + ToolTypeToOptions, ToolStaticOptions, BlockToolOptions, InlineToolOptions, BlockTuneOptions, + PreparedToolOptions +} from '../../entities'; import type { EditorAPI } from '../../api'; // eslint-disable-next-line @typescript-eslint/no-explicit-any -- need to allow any type here so extended interfaces pass @@ -109,6 +110,13 @@ export abstract class BaseToolFacade { + public prepare(): PreparedToolOptions | void | Promise { // eslint-disable-next-line @typescript-eslint/unbound-method if (isFunction(this.constructable.prepare)) { return this.constructable.prepare({ @@ -191,6 +199,23 @@ export abstract class BaseToolFacade { // } /** - * Returns Tool toolbox configuration (internal or user-specified). + * Returns Tool toolbox configuration (internal, config-derived, or user-specified). * - * Merges internal and user-defined toolbox configs based on the following rules: + * Resolved in three tiers, each layered on top of the previous: * - * - If both internal and user-defined toolbox configs are arrays their items are merged. - * Length of the second one is kept. + * 1. The static default (`constructable.options.toolbox`). + * 2. The config-derived value computed by the tool's `prepare()`, if any + * ({@link BaseToolFacade.preparedOptions}) — when present, it replaces the static default outright. + * 3. An explicit `use(Tool, options)` override, merged into whichever of the above applies + * using the rules below. + * + * Merging the resolved tool-side settings with a `use()` override follows these rules: + * + * - If both are arrays their items are merged. Length of the second one is kept. * * - If both are objects their properties are merged. * - * - If one is an object and another is an array than internal config is replaced with user-defined + * - If one is an object and another is an array than tool-side config is replaced with user-defined * config. This is made to allow user to override default tool's toolbox representation (single/multiple entries) + * + * `false` (from either the tool side or an explicit `use()` override) hides the tool from the + * toolbox entirely, unless a `use()` override supplies a real value on top of it. */ public get toolbox(): ToolboxConfigEntry[] | undefined { - const toolToolboxSettings = this.constructable.options?.[BlockToolOptionKey.Toolbox] as ToolboxConfig; + const toolToolboxSettings = ( + this.preparedOptions?.[BlockToolOptionKey.Toolbox] + ?? this.constructable.options?.[BlockToolOptionKey.Toolbox] + ) as ToolboxConfig | false | undefined; const userToolboxSettings = this.useToolOptions[UserToolOptions.Toolbox]; - if (isEmpty(toolToolboxSettings)) { + if (userToolboxSettings === false) { return; } - if (userToolboxSettings === false) { + if (!userToolboxSettings && (toolToolboxSettings === false || isEmpty(toolToolboxSettings))) { return; } /** * Return tool's toolbox settings if user settings are not defined */ if (!userToolboxSettings) { - return Array.isArray(toolToolboxSettings) ? toolToolboxSettings : [toolToolboxSettings]; + /** + * `toolToolboxSettings` can't be `false`/empty here — that was already returned above + */ + return Array.isArray(toolToolboxSettings) ? toolToolboxSettings : [toolToolboxSettings as ToolboxConfigEntry]; } /** diff --git a/packages/tools/paragraph/src/index.ts b/packages/tools/paragraph/src/index.ts index e9f56cb3..5fcf7732 100644 --- a/packages/tools/paragraph/src/index.ts +++ b/packages/tools/paragraph/src/index.ts @@ -1,11 +1,11 @@ -import type { ToolConfig } from '@editorjs/editorjs'; import type { BlockTool, BlockToolConstructor, BlockToolConstructorOptions, BlockToolData, KeyAddedEvent, - TextNodeSerialized + TextNodeSerialized, + ToolConfig } from '@editorjs/sdk'; import { KeyRemovedEvent } from '@editorjs/sdk'; import { ToolType } from '@editorjs/sdk'; From 2459752b386a5f004ba56b888c24aed40ca025de Mon Sep 17 00:00:00 2001 From: Reversean Date: Tue, 4 Aug 2026 23:29:51 +0300 Subject: [PATCH 3/6] feat(sdk): derive static tool options from tool config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool's `options` may now be a synchronous factory of its `ToolConfig`, resolved once per registration in the facade and never written back to the shared class. Static options were previously fixed at class-definition time, so config fields could not drive them — Header's `levels` had no way to produce its `toolbox` entries. `ToolConfig` also becomes an SDK-owned type instead of an `any`-defaulted re-export, which silently disabled checking for tools omitting the generic. BREAKING CHANGE: `ToolConfig` is exported from `@editorjs/sdk`, and `static options` widens to `Options | ((config) => Options)`. Co-Authored-By: Claude Opus 5 --- .../design.md | 99 +++++---- .../proposal.md | 26 +-- .../specs/sdk/spec.md | 28 +-- .../separate-tool-options-and-config/tasks.md | 56 ++--- packages/core/src/tools/ToolsManager.spec.ts | 153 -------------- packages/core/src/tools/ToolsManager.ts | 55 +---- packages/sdk/src/entities/BaseTool.spec.ts | 111 +++++----- packages/sdk/src/entities/BaseTool.ts | 58 ++++-- .../src/tools/facades/BaseToolFacade.spec.ts | 162 +++++++++------ .../sdk/src/tools/facades/BaseToolFacade.ts | 68 ++++--- .../src/tools/facades/BlockToolFacade.spec.ts | 192 ++++++++++++++++++ .../sdk/src/tools/facades/BlockToolFacade.ts | 42 ++-- 12 files changed, 575 insertions(+), 475 deletions(-) delete mode 100644 packages/core/src/tools/ToolsManager.spec.ts create mode 100644 packages/sdk/src/tools/facades/BlockToolFacade.spec.ts diff --git a/openspec/changes/separate-tool-options-and-config/design.md b/openspec/changes/separate-tool-options-and-config/design.md index 26ede70d..65ead24f 100644 --- a/openspec/changes/separate-tool-options-and-config/design.md +++ b/openspec/changes/separate-tool-options-and-config/design.md @@ -7,28 +7,28 @@ Today a tool's static surface is one bag (`ToolConstructor.options`) that mixes `ToolConfig` itself is not an SDK contract — it's re-exported from the legacy `@editorjs/editorjs` package as `type ToolConfig = T`, an untyped passthrough. A tool author gets whatever structure they declare in their own `Config` generic parameter, with nothing checking that it's consistent with anything else. -The concrete failure this produces is the Header tool's v3 migration (draft PR [editor-js/header#130](https://github.com/editor-js/header/pull/130)): `HeaderConfig.levels` is declared but never read anywhere — the static `options.toolbox` array is a hardcoded 3-entry list (H1–H3) that has no way to depend on `config.levels`, because `toolbox` is evaluated once, statically, at class-definition time, while `config` is only resolved per tool-registration (in `BaseToolFacade.config`) and per block instance (in the constructor). - -The lifecycle already has a hook positioned exactly where this could be fixed: `ToolsManager.prepareTools()` calls `toolConstructor.prepare({ toolName, config: tool.config })` — with `tool.config` already the fully merged config — and only *after* that call resolves does it call `setToAvailableToolsCollection`, which dispatches `ToolLoadedCoreEvent`. `ToolboxUI` only starts reading a tool's `toolbox` getter in response to that event. So `prepare()` already runs at the right time, with the right data; it just isn't a channel that can influence `options` today, and nothing tells a tool author it's meant to be. +The concrete failure this produces is the Header tool's v3 migration (draft PR [editor-js/header#130](https://github.com/editor-js/header/pull/130)): `HeaderConfig.levels` is declared but never read anywhere — the static `options.toolbox` array is a hardcoded 3-entry list (H1–H3) that has no way to depend on `config.levels`, because `options` is evaluated once, statically, at class-definition time, while `config` is only resolved per tool-registration (in `BaseToolFacade.config`) and per block instance (in the constructor). Constraints this design has to respect: - A page can run multiple `Core` instances that both `use()` the same tool class (e.g. two editors sharing one `Header` import). Anything a tool computes from its own resolved config must not be written onto the shared class/static object, or the second instance's config would clobber the first's. -- Per this change's proposal, a breaking change to `ToolConfig`'s type/import is acceptable; a live "change config after mount" API is explicitly out of scope. -- The project's existing TDD convention applies to the facade/manager changes below (see `openspec/config.yaml` rules). +- Every consumer of a tool's static options reads it through a **synchronous** getter (`BlockToolFacade.toolbox`, `isReadOnlySupported`, `conversionConfig`, `InlineToolFacade`'s option reads). Whatever produces those values must therefore be resolvable synchronously by the time the facade exists. +- Per this change's proposal, a breaking change to `ToolConfig`'s type/import and to the `static options` type is acceptable; a live "change config after mount" API is explicitly out of scope. +- The project's existing TDD convention applies to the facade changes below (see `openspec/config.yaml` rules). ## Goals / Non-Goals **Goals:** - Make `ToolConfig` an SDK-owned, real contract (not a re-exported `any`-defaulted passthrough). - Keep `ToolOptions` (`BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions`) as the static, core/plugin-facing contract, formally distinct from `ToolConfig`. -- Let a tool compute config-derived static options (starting with `toolbox`) from its fully-resolved `ToolConfig`, resolved once during tool preparation, via the existing `prepare()` hook — without mutating the tool's shared static class property. -- Catch the `HeaderConfig.levels`-style failure mode: a config key that's declared but never actually consumed. +- Let a tool derive its static options from its `ToolConfig` — without mutating the tool's shared static class property, and without adding a resolution step to the tool-registration lifecycle. +- Keep the change's blast radius inside the SDK: no new plumbing in `packages/core`. **Non-Goals:** -- No live/reactive config updates after the editor has mounted (confirmed out of scope — config is resolved once, during preparation). -- No change to how `shortcut`, `inlineToolbar`, or `tunes` are merged — this change only adds the config-derived tier to `toolbox`, since that's the concrete, evidenced need; the same mechanism can be extended to other option fields later if a real case shows up. -- Not migrating the `header` tool itself — it lives in a separate repo/submodule. This change ships the SDK mechanism; wiring `Header.prepare()` to use it is a follow-up in that repo. -- No new runtime schema-validation dependency (e.g. zod/io-ts). +- No live/reactive config updates after the editor has mounted (confirmed out of scope — config is resolved once, at facade construction). +- No change to `prepare()`. Async, side-effectful tool initialization keeps working exactly as it does today. +- No change to how `toolbox`, `shortcut`, `inlineToolbar`, or `tunes` are merged with `use()`-time overrides — the merge algorithm and its tiers stay as they are. +- No dev-time "declared but unused config key" diagnostic in this change (see Decision 8). +- Not migrating the `header` tool itself — it lives in a separate repo/submodule. This change ships the SDK mechanism; wiring `Header.options` to use it is a follow-up in that repo. ## Decisions @@ -37,45 +37,74 @@ The legacy `ToolConfig = T` is how the untyped-escape-ha *Alternative considered*: leave the re-export and only tighten the `Config extends ToolConfig` bound on each interface (`BlockToolOptions`, etc.). Rejected — the default still resolves to `any` for any tool that skips the generic, which is exactly today's failure mode. **2. `options` and `config` stay two separate top-level concepts; `options.config` remains the only bridge.** -This matches how they're actually consumed: `options` (via facade getters) is read by core/plugins before any block exists; `config` is read by the tool instance itself. Collapsing them into one `ToolSettings` bag would erase that timing distinction, which is the actual source of the bug (something that's plugin-timing data, like `toolbox`, has no way to see something that's tool-instance-timing data, like `config.levels`, unless the two are explicitly bridged). +This matches how they're actually consumed: `options` (via facade getters) is read by core/plugins before any block exists; `config` is read by the tool instance itself. Collapsing them into one `ToolSettings` bag would erase that timing distinction, which is the actual source of the bug. *Alternative considered*: a single unified settings object passed everywhere. Rejected — every plugin consumer would need to filter out user-data noise, and it doesn't resolve the "when is this available" question. -**3. Config-derived options are resolved via `prepare()`'s return value, held per-facade-instance — not by writing onto `ClassName.options`.** -`prepare()`'s signature extends to: +**3. Config-derived options are expressed by letting `options` itself be a function of `config` — not by routing them through `prepare()`'s return value.** +`BaseToolConstructor.options` widens to: ```ts -prepare?(data: { toolName: string, config: Config }): - PreparedToolOptions | void | Promise; +type ToolOptionsFactory = (config: Config) => Options; + +options?: Options | ToolOptionsFactory; ``` -where `PreparedToolOptions` is a partial type restricted, for now, to `{ toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false }`. `ToolsManager.prepareTools()` captures this return value and passes it through `ToolsFactory` into the facade constructor as a new `preparedOptions` field, stored on the `BaseToolFacade` instance (not on `constructable`). -*Alternative considered*: have the tool mutate `ClassName.options` directly inside `prepare()` (the shape floated during earlier discussion of this change). Rejected once the multi-`Core`-instance case was worked through: `constructable.options` is one object shared by every facade wrapping that class, so a second `Core` instance's `prepare()` call would overwrite the first instance's computed toolbox for both. Returning a value and letting the facade own it keeps the result properly scoped per editor instance while still using the same trigger point (`prepare()`) already agreed on. +The two hooks answer different questions and have different natures. `prepare()` is *asynchronous, side-effectful initialization* — its v2 role is loading external resources before the tool can be used, and it is called for its effects, not its value. Deriving `toolbox` entries from `config.levels` is a *pure, synchronous* mapping over data the framework already holds. Merging the two conflates those natures, and a chain of machinery follows from the merge: because `prepare()` may be async, its result cannot be read by the synchronous option getters, so it must be resolved eagerly and cached behind a dedicated type and a facade slot, and that caching must in turn be sequenced by `ToolsManager` ahead of `ToolLoadedCoreEvent`. Keeping the derivation on `options` itself makes all of it unnecessary. +*Alternative considered*: widen `prepare()` to return a `PreparedToolOptions` object that the facade stores and its option getters consult. Rejected on the above; concretely it costs a new exported type, three new members on `BaseToolFacade` (a private field, a getter, a setter), capture-and-inject logic in `ToolsManager.prepareTools()`, an ordering guarantee relative to `ToolLoadedCoreEvent` that anyone touching that method must preserve, and an allow-list of derivable fields — starting at `toolbox` alone — that has to be widened by hand for every further field. The factory form derives the whole option object with none of it. +*Alternative considered*: a second static hook alongside `options`, e.g. `resolveOptions(config)`. Rejected — it keeps two declaration sites for one concept, which is the thing the review objected to; the union type on a single `options` member has one declaration site and one resolution site. + +**4. The factory receives the `use()`-time config, and applies its own defaults inside its body.** +The argument is `useToolOptions.config ?? {}` — exactly the object an integrator passed to `core.use(Tool, { config })`. A tool applies its defaults where it derives from them: +```ts +static options = (config: HeaderConfig) => { + const levels = config.levels ?? [1, 2, 3]; + + return { + config: { levels, defaultLevel: config.defaultLevel ?? 2 }, + toolbox: levels.map(level => ({ title: `Heading ${level}`, icon: ICONS[level], data: { level } })), + }; +}; +``` +`BaseToolFacade.config` then merges as it always has — `{ ...resolvedOptions.config, ...useToolOptions.config }` — so the tool *instance* still receives a config carrying the defaults, and the object form of `options` behaves byte-for-byte as it does today. +*Alternative considered*: pass the fully merged config (tool-side defaults ∪ `use()`-time override) into the factory, mirroring what `prepare()` receives today. Rejected as genuinely circular: the tool-side defaults live in `options.config`, which for a factory-form tool only exists in that factory's own return value, so building its input would require its output. +*Trade-off accepted*: for factory-form tools the framework no longer pre-merges `options.config` defaults *before* the derivation runs — the tool writes `?? default` itself. In exchange the defaults live in exactly one place in the tool's source, next to the code that consumes them, instead of being split between a static `options.config` block and the logic that reads it. + +**5. The factory is synchronous.** +Every consumer of static options is a synchronous getter, so allowing a `Promise` return would reintroduce precisely the eager-resolve-and-cache problem Decision 3 removes: `ToolsManager` would again have to await the value and inject it before `ToolLoadedCoreEvent`, and the facade would again need a "not resolved yet" state. A tool that needs asynchronous work before it can operate still has `prepare()`; that work simply cannot feed the tool's static declaration. +*Alternative considered*: `options?: Options | ((config) => Options | Promise)`. Rejected — no evidenced case (nothing in-repo, and the motivating Header case is a pure array map), and it costs the entire simplification. + +**6. The factory is resolved once, in the facade constructor, into a private per-instance field.** +`BaseToolFacade` computes `isFunction(constructable.options) ? constructable.options(useConfig) : (constructable.options ?? {})` at construction time and keeps the result in a private field. Every option-reading getter — `options` and `config` on `BaseToolFacade`, `toolbox` and `isReadOnlySupported` on `BlockToolFacade` — reads that field rather than `constructable.options`. Those four getters are the only places in the repo that read a tool class's static options directly, so the conversion is fully enumerable. +This makes the multi-`Core` constraint hold *by construction* rather than by mitigation: the factory is a pure function on the class, each facade calls it with its own `useToolOptions.config`, and nothing is ever written back to the shared `constructable`. Two `Core` instances using one `Header` import with different `levels` cannot interfere. +*Alternative considered*: resolve lazily and memoize on first read. Rejected as equivalent in effect but worse in failure mode — a throwing factory would surface at an arbitrary getter read rather than at tool registration. +*Alternative considered*: call the factory on every getter read. Rejected — a tool author would reasonably assume a single call, and repeated calls would make `toolbox` entry identity unstable across reads. -**4. `toolbox` resolution gains a third tier, in this order: static default → prepared (config-derived) → explicit `use()`-time override.** -`BlockToolFacade.toolbox` keeps its existing array/object positional-merge algorithm; it's just seeded from `preparedOptions.toolbox ?? constructable.options.toolbox` instead of `constructable.options.toolbox` alone. All current explicit-override behavior (`toolbox: false` to hide, partial per-entry overrides via `use()`) is preserved unchanged, now layered on top of whatever the tool computed from its own config. -*Alternative considered*: make `preparedOptions.toolbox` and `useToolOptions.toolbox` mutually exclusive. Rejected as an unnecessary behavior cliff — keeping one uniform merge means the new tier is additive rather than a special case integrators need to remember. +**7. `toolbox` resolution keeps its existing two tiers and its existing merge algorithm.** +`BlockToolFacade.toolbox` still resolves tool-side value, then the explicit `use()`-time override on top, with the same array/object positional-merge rules and the same `toolbox: false` hiding behavior. Only the *source* of the tool-side value changes, from `constructable.options.toolbox` to the resolved static options. The factory result replaces what the static object would have been; it is not an extra layer. +*Alternative considered*: a third tier, in which a separately-derived toolbox value takes precedence over the static one before the `use()` override is applied on top (`derived ?? static`, then `use()`). Rejected with Decision 3 — a third tier is only needed when derivation arrives through a channel *alongside* `options`; when the factory *is* `options`, there is nothing left to layer. -**5. Verification is a dev-time reachability check on the resolved config object, not a schema-validation library.** -A lightweight check (gated the same way existing `ToolsManager` dev diagnostics are — `console.warn`, not thrown, and skippable in production builds) flags a key present in a tool's *resolved* `ToolConfig` object that is never read: neither passed through via `options.config` defaults, nor touched by a `prepare()` that returns `PreparedToolOptions`. This is a reachability check on real objects and function calls at runtime, not a compile-time-only type comparison (TS types don't exist at runtime) — so it directly catches the `HeaderConfig.levels`-declared-but-unused case without requiring a new dependency. -*Alternative considered*: full schema validation of `ToolConfig` shapes (zod/io-ts). Rejected as disproportionate to the actual failure mode, which is "declared but never read," not "wrong shape." +**8. Detecting a declared-but-unused config key is out of scope for this change.** +A tempting companion feature is a dev-time warning when a tool declares a `ToolConfig` key that nothing ever reads — the `levels` drift would then announce itself instead of failing silently. The obvious implementation, wrapping the resolved config in a `Proxy` that records key access during tool registration, does not survive contact with the lifecycle: the only registration-time hook a tool exposes is `prepare()`, which most tools — including all four in-repo ones — do not define at all, so the check would simply never run for them; and a key read in the tool's *constructor*, which is exactly where Header reads `defaultLevel`, happens per block instance, after any registration-time observation window has closed, so it would be reported as unused. Getting this right needs a reachability model — a decision about what "used" means — that this change does not otherwise require, and the motivating bug is fixed by making `levels` *derivable*, not by warning about it. +*Alternative considered*: keep it and fix it in place (run for all tools, treat any config read as use). Rejected as scope creep onto a contract change — it needs its own design discussion about the reachability model. -**6. Tools without a `prepare()` method are unaffected at runtime.** -`bold`, `italic`, `inline-link`, and `paragraph` don't currently need config-derived options; their migration is limited to importing the new SDK-owned `ToolConfig` type. Even though a breaking change is acceptable per this change's scope, there's no reason to force a runtime-behavior migration where only the type source moved. +**9. Tools that keep the object form of `options` are unaffected at runtime.** +`bold`, `italic`, `inline-link`, and `paragraph` all declare `static readonly options = { ... }` and none needs config-derived options; their migration is limited to importing the new SDK-owned `ToolConfig` type. The union type is additive — the object branch is the existing behavior. ## Risks / Trade-offs -- **[Risk]** A third merge tier (static → prepared → `use()`-override) adds a step to an already non-trivial merge chain in `BlockToolFacade.toolbox`, raising the bar for contributors reading it. → **Mitigation**: keep all three tiers resolved in one getter (as today), document them with a spec scenario (see delta spec), and add a facade unit test per tier combination (static-only, prepared-only, both, both-plus-explicit-override). -- **[Risk]** Widening `prepare()`'s return type is itself a breaking change for any hypothetical tool already using `prepare()` for pure side effects that happens to return a truthy non-`undefined` value. → **Mitigation**: only recognized keys (`toolbox`, for now) are read off the returned object; anything else is ignored with a dev-time warning rather than silently applied or throwing. -- **[Risk]** The "unused config key" check can only see what's reachable at runtime, not a tool's full declared type (erased at compile time), so it can miss a key that exists in the `Config` type but was never included in a given call's resolved config object. → **Mitigation**: scope the check's guarantee accordingly — it catches "this resolved config has a key nothing reads," which is exactly the Header failure mode, not "this type has a key that's structurally unreachable." -- **[Risk]** The validating real-world case (`editor-js/header#130`) lives outside this repo and could merge with the bug still present before this change ships its mechanism. → **Mitigation**: tasks.md treats that PR as an acceptance reference, not a task owned by this change; wiring `Header.prepare()` to the new mechanism is explicitly a follow-up in the `header` repo. +- **[Risk]** `options` becoming a union (object or factory) means any code reading it must narrow first, and a tool's static options can no longer be inspected without invoking the factory. → **Mitigation**: resolution is centralized in one place (the `BaseToolFacade` constructor) and the 5 existing read sites are converted to read the resolved field; the facade is the sanctioned read path for everything outside the tool itself, and there is no in-repo consumer that reads `Tool.options` directly off a class. +- **[Risk]** For factory-form tools the framework no longer merges `options.config` defaults into the value handed to the derivation, so a tool author who forgets a `?? default` gets `undefined` where the object form would have given them a default. → **Mitigation**: the factory's `config` parameter is typed as the tool's own `Config`, so the optionality is visible at the call site; and the defaults the factory returns under `config` still flow to the tool instance through the unchanged `BaseToolFacade.config` merge. +- **[Risk]** Sync-only factories mean a genuinely async-derived option (e.g. a toolbox built from a fetched preset list) is not expressible. → **Mitigation**: no such case exists in-repo or in the motivating PR; if one appears, revisiting Decision 5 is a contained change (the facade would need a resolved/unresolved state and `ToolsManager` an await point) rather than a redesign. +- **[Risk]** The validating real-world case (`editor-js/header#130`) lives outside this repo and could merge with the bug still present before this change ships its mechanism. → **Mitigation**: tasks.md treats that PR as an acceptance reference, not a task owned by this change; wiring `Header.options` to the new form is explicitly a follow-up in the `header` repo. ## Migration Plan -- Ship the SDK contract changes (`ToolConfig`, `ToolOptions`, `prepare()` signature), facade changes, and `ToolsManager`/`ToolsFactory` plumbing together — they're tightly coupled; a partial rollout would leave the new `prepare()` return type with no consumer. +- Ship the SDK contract changes (`ToolConfig`, the widened `options` type) and the facade resolution together — they are tightly coupled; a partial rollout would leave the factory form declared in the types with no resolver behind it. - Update the four in-repo tools' `Config` type imports (mechanical, type-only, no runtime behavior change). - No feature flag or staged rollout: this is a pre-1.0 internal SDK surface, and per this change's confirmed scope a breaking change is acceptable with no external in-repo consumers beyond the tools already covered. -- Rollback: revert the SDK/facade/core commits together; the tools' type-only import changes revert cleanly since they carry no runtime behavior. +- Rollback: revert the SDK/facade commits together; the tools' type-only import changes revert cleanly since they carry no runtime behavior. ## Open Questions -- Should `PreparedToolOptions` generalize beyond `toolbox` (e.g. `shortcut`, `inlineToolbar`) once a second concrete need appears, or stay toolbox-only indefinitely? Leaning toolbox-only until evidenced otherwise (YAGNI). -- Should the "unused config key" dev warning live in SDK core (always on outside production, matching existing `ToolsManager` console diagnostics) or as an opt-in lint/test helper tool authors run in CI? Leaning SDK-core for now; worth revisiting if it proves noisy. -- How would a third-party plugin (i.e. not one of the framework's own Toolbox/Shortcuts/InlineToolbar/Tunes consumers) read its own config-derived data off a tool? Today `options`/`PreparedToolOptions` are a closed, core-defined schema, not an extensible registry a third party can add fields to. `BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions` already carry a `[key: string]: unknown` escape hatch for custom static fields a tool author reads back themselves; `PreparedToolOptions` currently doesn't. Not adding that now (no evidenced consumer) — worth revisiting together with the bullet above if a real third-party case shows up. +- Should the factory also receive `toolName`, as `prepare()` does (`(config, toolName) => Options` or an object argument)? Leaning no — no evidenced need, and a single positional `config` argument keeps the signature readable; the object-argument form is the escape hatch if a second input ever appears. +- `BlockToolFacade.isReadOnlySupported` reads the tool-side options only, so a `use()`-time override cannot enable/disable it. This change relocates that read to the resolved static options but preserves the behavior. Whether it *should* honor a `use()` override is a separate question, deliberately not answered here. +- Should "unused config key" detection (Decision 8) become its own proposal, and if so, what counts as a key being used — read by the options factory, by `prepare()`, or by the tool's constructor? The constructor case is the hard one, since it is per block instance rather than per registration. +- How would a third-party plugin (i.e. not one of the framework's own Toolbox/Shortcuts/InlineToolbar/Tunes consumers) read its own config-derived data off a tool? `BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions` already carry a `[key: string]: unknown` escape hatch for custom static fields, and the factory form derives those alongside everything else — so this is arguably now answered, but no third-party case has been tested against it. diff --git a/openspec/changes/separate-tool-options-and-config/proposal.md b/openspec/changes/separate-tool-options-and-config/proposal.md index 3f69b89f..5c76b212 100644 --- a/openspec/changes/separate-tool-options-and-config/proposal.md +++ b/openspec/changes/separate-tool-options-and-config/proposal.md @@ -1,17 +1,18 @@ ## Why -Tools expose two static surfaces with no formal separation: `static options` (core/plugin-facing wiring — `toolbox`, `shortcut`, `inlineToolbar`, `tunes`, `conversionConfig`, `canBeSplit`) and `ToolConfig` (plugin-specific user data, typed only via a generic `Config extends ToolConfig = any` re-exported from the legacy `@editorjs/editorjs` package, where `ToolConfig = T` is an untyped passthrough). Because `options.toolbox` is a plain static value evaluated once at class-definition time, a tool has no way to express "toolbox entries depend on my resolved config" — there is no formal contract, no type checking, and no runtime verification connecting a `ToolConfig` field to the `options` it's meant to drive. +Tools expose two static surfaces with no formal separation: `static options` (core/plugin-facing wiring — `toolbox`, `shortcut`, `inlineToolbar`, `tunes`, `conversionConfig`, `canBeSplit`) and `ToolConfig` (plugin-specific user data, typed only via a generic `Config extends ToolConfig = any` re-exported from the legacy `@editorjs/editorjs` package, where `ToolConfig = T` is an untyped passthrough). Because `options` is a plain static value evaluated once at class-definition time, a tool has no way to express "these option values are derived from my resolved config" — there is no contract connecting a `ToolConfig` field to the `options` it is meant to drive. -This is not hypothetical: it is live today in the Header tool's v3 SDK migration (draft PR [editor-js/header#130](https://github.com/editor-js/header/pull/130)). `HeaderConfig` declares `levels` and `defaultLevel`, and `defaultLevel` does flow through (it's read inside the constructor from the resolved `config` object). But `levels` is dead — the static `options.toolbox` array is hardcoded to exactly three entries (H1–H3) and never consults `config.levels`, so a user who configures `levels: [1]` sees no change in the toolbox. The mechanism to fix this already exists in the core tool lifecycle (`prepare({ toolName, config })` runs once per registered tool, before the tool is announced to the Toolbox/Shortcuts/InlineToolbar/Tunes plugins), but nothing in the SDK's contracts guides a tool author to use it for config-derived options, and nothing catches the drift when, as in Header, a declared config field ends up unused. +This is not hypothetical: it is live today in the Header tool's v3 SDK migration (draft PR [editor-js/header#130](https://github.com/editor-js/header/pull/130)). `HeaderConfig` declares `levels` and `defaultLevel`; `defaultLevel` flows through (it is read inside the constructor from the resolved `config` object), but `levels` is dead — the static `options.toolbox` array is hardcoded to exactly three entries (H1–H3) and never consults `config.levels`, so a user who configures `levels: [1]` sees no change in the toolbox. ## What Changes - Define `ToolConfig` as an SDK-owned, per-tool-type-parameterized contract, replacing the untyped passthrough currently imported from `@editorjs/editorjs`. **BREAKING**: a tool's `Config` generic must conform to the new contract's shape and import path. -- Keep `ToolOptions` (`BlockToolOptions` / `InlineToolOptions` / `BlockTuneOptions`) as the static, declarative, core/plugin-facing contract, formally separated from `ToolConfig` — `options.config` remains the channel for config *defaults*, but is no longer the only thing standing in for "everything a tool needs at runtime". -- Extend the `prepare()` contract so a tool can return config-derived option values (starting with `toolbox`) after receiving its fully-resolved `ToolConfig`, and have the result flow into the tool's effective options before it is advertised to plugins — without mutating the tool's shared static class property (the current pattern of writing directly to `ClassName.options` would leak across multiple `Core` instances sharing the same tool class on one page). -- Add compile-time typing and a dev-time check that flags a `ToolConfig` field which no computed option / consumer reads (directly addressing the `HeaderConfig.levels` drift). -- Migrate the four in-repo tools (`paragraph`, `bold`, `italic`, `inline-link`) to the new `ToolConfig`/`ToolOptions` contracts. **BREAKING** for any tool relying on the current plain-object-only `static options` typing. -- Out of scope: changing a tool's config after the editor has already mounted (no live/reactive "hot-swap" API). Config is resolved once, during tool preparation, before the editor renders its UI. +- Allow a tool's static `options` to be, in addition to a plain object, a **synchronous factory** `(config: ToolConfig) => ToolOptions`. The factory receives the `config` supplied at `use(Tool, { config })` time and returns the tool's complete static option set — including any config defaults it applies, under `options.config`, exactly as the object form declares them. **BREAKING** for the `static options` type of every tool. +- Resolve that factory exactly once, when the tool's facade is constructed, into a private per-facade field. Every option-reading getter (`options`, `config`, `toolbox`, `isReadOnlySupported`) reads the resolved value instead of `constructable.options`, so nothing is ever written back onto the shared tool class. +- Explicitly **do not** change `prepare()`. It stays the `void`-returning, optionally-async, side-effectful initialization hook it is today; deriving static options from config is a separate, pure, synchronous concern and is not routed through it. +- Migrate the four in-repo tools (`paragraph`, `bold`, `italic`, `inline-link`) to the new `ToolConfig`/`ToolOptions` contracts. All four keep the plain-object form of `options`; none needs config-derived options. +- Out of scope: changing a tool's config after the editor has already mounted (no live/reactive "hot-swap" API). Config is resolved once, during tool registration, before the editor renders its UI. +- Out of scope: dev-time detection of a declared-but-unused `ToolConfig` key. It is a distinct diagnostics concern with its own open question (what counts as "used" — a key read in a factory, in `prepare()`, or in the tool's constructor?) and does not belong in the contract change. ## Capabilities @@ -19,13 +20,14 @@ This is not hypothetical: it is live today in the Header tool's v3 SDK migration (none — this reshapes the existing tool-contract behavior rather than introducing a new capability area) ### Modified Capabilities -- `sdk`: the "Tool and tune contracts" requirement changes — `ToolConfig` becomes a dedicated SDK contract (no longer a passthrough re-export), the static `options`/`config` merge behavior gains a "config-derived options resolved via `prepare()`" step that runs before static options are read by consumers (Toolbox, Shortcuts, InlineToolbar, Tunes), and `BaseToolConstructor.prepare()`'s signature/return type changes accordingly. +- `sdk`: the "Tool and tune contracts" requirement changes — `ToolConfig` becomes a dedicated SDK contract (no longer a passthrough re-export), and `BaseToolConstructor.options` widens from a plain object to "a plain object **or** a synchronous factory of the resolved config", with the facade owning a single resolution of that factory per registration. ## Impact -- `packages/sdk/src/entities/{BaseTool.ts, BlockTool.ts, InlineTool.ts, BlockTune.ts}`: new `ToolConfig`/`ToolOptions` contracts; extended `prepare()` signature and return type. -- `packages/sdk/src/tools/facades/{BaseToolFacade.ts, BlockToolFacade.ts, InlineToolFacade.ts, BlockTuneFacade.ts}`: a per-facade-instance slot for `prepare()`-computed options, inserted into the existing static/`use()`-time merge chain (the `toolbox` getter's merge algorithm gains a tier between static defaults and explicit `use()`-time overrides). -- `packages/core/src/tools/{ToolsManager.ts, ToolsFactory.ts}`: capture `prepare()`'s return value and thread computed options into facade construction, ahead of the `ToolLoadedCoreEvent` dispatch that Toolbox/etc. listen for. -- `packages/tools/{paragraph,bold,italic,inline-link}`: migrate to the new `ToolConfig` import/contract (type-only change; no tool here currently needs config-derived options). +- `packages/sdk/src/entities/{BaseTool.ts, BlockTool.ts, InlineTool.ts, BlockTune.ts}`: SDK-owned `ToolConfig`; `options` widened to accept a config factory. `prepare()`'s signature is unchanged. +- `packages/sdk/src/tools/facades/BaseToolFacade.ts`: a private per-instance field holding the resolved static options, populated in the constructor; the `options` and `config` getters read it instead of `constructable.options`. +- `packages/sdk/src/tools/facades/BlockToolFacade.ts`: the `toolbox` and `isReadOnlySupported` getters read the resolved static options. The `toolbox` merge algorithm and its two tiers (tool-side value, then `use()`-time override) are unchanged. +- `packages/core/src/tools/ToolsManager.ts`: unchanged. Resolution happens entirely inside the SDK facade, so the tool-registration lifecycle gains no new step and no new ordering guarantee. +- `packages/tools/{paragraph,bold,italic,inline-link}`: migrate to the new `ToolConfig` import/contract (type-only change; all keep object-form `options`). - `editor-js/header` (external submodule repo, PR #130): not modified by this change directly, but is the motivating and validating case — its `HeaderConfig.levels` drift is the concrete bug this change makes fixable. - `openspec/specs/sdk/spec.md`: delta spec updates to the "Tool and tune contracts" requirement. diff --git a/openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md b/openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md index 64c3e6d6..218fb37f 100644 --- a/openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md +++ b/openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md @@ -1,27 +1,27 @@ ## MODIFIED Requirements ### Requirement: Tool and tune contracts -The system SHALL define the static/instance contracts that block tools, inline tools, and block tunes must satisfy: `BaseTool`/`BaseToolConstructor` (common `name`, `options`, `prepare()`, `reset()`), `BlockTool`/`BlockToolConstructor` (adds `toolbox`, `shortcut`, `inlineToolbar`, `tunes`, `conversionConfig`, `canBeSplit`), `InlineTool`/`InlineToolConstructor` (adds `isActive`, `getFormattingOptions`, `createWrapper`, `getToolbarConfig`), and `BlockTune`/`BlockTuneConstructor`. `ToolConfig` SHALL be an SDK-owned contract (not a re-exported passthrough from `@editorjs/editorjs`), kept formally distinct from `ToolOptions`: `options` describes core/plugin-facing wiring available before any block instance exists, while `ToolConfig` describes tool-author-facing user data resolved per tool registration. +The system SHALL define the static/instance contracts that block tools, inline tools, and block tunes must satisfy: `BaseTool`/`BaseToolConstructor` (common `name`, `options`, `prepare()`, `reset()`), `BlockTool`/`BlockToolConstructor` (adds `toolbox`, `shortcut`, `inlineToolbar`, `tunes`, `conversionConfig`, `canBeSplit`), `InlineTool`/`InlineToolConstructor` (adds `isActive`, `getFormattingOptions`, `createWrapper`, `getToolbarConfig`), and `BlockTune`/`BlockTuneConstructor`. `ToolConfig` SHALL be an SDK-owned contract (not a re-exported passthrough from `@editorjs/editorjs`), kept formally distinct from `ToolOptions`: `options` describes core/plugin-facing wiring available before any block instance exists, while `ToolConfig` describes tool-author-facing user data resolved per tool registration. A tool's static `options` SHALL be either a plain options object or a synchronous factory taking the tool's `ToolConfig` and returning that options object; the facade SHALL resolve the factory exactly once, at construction, and serve every option-reading getter from that resolved value without writing it back onto the tool class. #### Scenario: Options and config merging in a tool facade - **GIVEN** a tool class has static `options` (and optionally `options.config` typed as `ToolConfig`) - **WHEN** the tool is registered via `use(Tool, options)` with overriding options -- **THEN** the facade's `options` getter merges static options with `use()`-time options, with `use()`-time keys taking precedence, and the `config` getter merges similarly, injecting `defaultPlaceholder` only when `isDefault` is true and no `placeholder` key is already present +- **THEN** the facade's `options` getter merges the tool's resolved static options with `use()`-time options, with `use()`-time keys taking precedence, and the `config` getter merges similarly, injecting `defaultPlaceholder` only when `isDefault` is true and no `placeholder` key is already present -#### Scenario: Resolving config-derived toolbox entries via prepare() -- **GIVEN** a block tool's static `prepare({ toolName, config })` returns an object containing a `toolbox` field -- **WHEN** the tool is being registered, before `ToolLoadedCoreEvent` is dispatched for it -- **THEN** the returned `toolbox` value is captured on the tool's facade instance — not written onto the tool's shared static `options` object — and is available to the facade's `toolbox` getter ahead of the tool being announced to the Toolbox/Shortcuts/InlineToolbar/Tunes plugins +#### Scenario: Resolving static options from a config factory +- **GIVEN** a tool class declares `static options` as a function of its `ToolConfig` rather than as a plain object +- **WHEN** its facade is constructed for a registration made via `use(Tool, { config })` +- **THEN** the function is invoked exactly once with the `config` supplied at `use()` time, its returned options object becomes the tool's resolved static options for that facade, and the tool class's own `options` property is left untouched -#### Scenario: Toolbox merge tiers -- **GIVEN** a block tool has a static `options.toolbox` default, a `prepare()`-computed `toolbox` value, and/or a `use()`-time `toolbox` override +#### Scenario: Deriving a toolbox entry from a config value +- **GIVEN** a block tool's `options` factory computes `toolbox` entries from a field of the config it receives - **WHEN** the facade's `toolbox` getter is read -- **THEN** the value is resolved in order — static default, then the `prepare()`-computed value if present, then the explicit `use()`-time override on top — using the existing array/object positional-merge algorithm, and a `use()`-time `toolbox: false` still hides the tool from the toolbox regardless of any computed value +- **THEN** the entries reflect the config supplied at `use()` time, and any explicit `use()`-time `toolbox` override is merged on top using the existing array/object positional-merge algorithm, with a `use()`-time `toolbox: false` still hiding the tool from the toolbox -#### Scenario: Detecting an unused config key -- **GIVEN** a tool's resolved `ToolConfig` object has a key that is neither passed through via `options.config` defaults nor read by a `prepare()` that returns a `toolbox` (or other recognized) field -- **WHEN** the tool finishes preparation -- **THEN** the system emits a dev-time warning identifying the unused key, without throwing or blocking tool registration +#### Scenario: Isolating factory-derived options per facade instance +- **GIVEN** two `Core` instances each register the same tool class through `use()` with a different `config` +- **WHEN** each instance's facade resolves the tool's `options` factory +- **THEN** each facade holds its own resolved options and neither instance's derived values are observable from the other #### Scenario: Text content conversion without config - **GIVEN** a block tool has no `conversionConfig` @@ -33,4 +33,4 @@ The system SHALL define the static/instance contracts that block tools, inline t - **WHEN** `exportTextContent`/`importTextContent` is called - **THEN** the value at that keypath is read/written, producing or consuming a `TextNodeSerialized` value tagged with the hidden `Text` block-child-type marker -Implemented in `src/entities/BaseTool.ts`, `BlockTool.ts`, `InlineTool.ts`, `BlockTune.ts`, `src/tools/facades/{BaseToolFacade,BlockToolFacade,InlineToolFacade,BlockTuneFacade}.ts`, `packages/core/src/tools/{ToolsManager.ts,ToolsFactory.ts}`, validated by `src/tools/facades/BaseToolFacade.spec.ts`. +Implemented in `src/entities/BaseTool.ts`, `BlockTool.ts`, `InlineTool.ts`, `BlockTune.ts`, `src/tools/facades/{BaseToolFacade,BlockToolFacade,InlineToolFacade,BlockTuneFacade}.ts`, validated by `src/tools/facades/BaseToolFacade.spec.ts`. diff --git a/openspec/changes/separate-tool-options-and-config/tasks.md b/openspec/changes/separate-tool-options-and-config/tasks.md index 3fc43580..1969d182 100644 --- a/openspec/changes/separate-tool-options-and-config/tasks.md +++ b/openspec/changes/separate-tool-options-and-config/tasks.md @@ -1,36 +1,42 @@ -## 1. SDK contracts: `ToolConfig` and `ToolOptions` +## 1. SDK-owned `ToolConfig` -- [x] 1.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` asserting `ToolConfig` is exported from `@editorjs/sdk` itself (not re-exported from `@editorjs/editorjs`), and that `BaseToolOptions` references the SDK-owned type — scoped to `BaseToolOptions` only rather than repeating the same check on `BlockToolOptions`/`InlineToolOptions`/`BlockTuneOptions`, since none of them redeclare the `config` field (they inherit it unmodified from `BaseToolOptions`), so per-subtype tests would exercise the identical TS structural-inheritance check with no added coverage -- [x] 1.2 Define `ToolConfig` in `packages/sdk/src/entities/BaseTool.ts` as an SDK-owned generic type (replacing the `@editorjs/editorjs` re-export), update `BaseToolOptions` and all per-tool-type option interfaces (`BlockTool.ts`, `InlineTool.ts`, `BlockTune.ts`) to import it from there -- [x] 1.3 Update `packages/tools/{paragraph,bold,italic,inline-link}` to import `ToolConfig`/their `Config` type from the new SDK location; confirm `yarn workspace typecheck`/`yarn lint` pass with no behavior change +- [x] 1.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` — `it('should expose ToolConfig from the SDK itself rather than re-export it from @editorjs/editorjs')` — asserting `BaseToolOptions` references the SDK-owned type +- [x] 1.2 Define `ToolConfig` in `packages/sdk/src/entities/BaseTool.ts` as an SDK-owned generic type, replacing the `@editorjs/editorjs` re-export, and update `BaseToolOptions` +- [x] 1.3 Update `BlockTool.ts`, `InlineTool.ts`, and `BlockTune.ts` to import `ToolConfig` from the new location — each re-declares its own `Config extends ToolConfig = ToolConfig` bound rather than inheriting `BaseToolOptions`' one, so this is covered by its own test (`should keep the config option checked on every tool subtype`) -## 2. `prepare()` contract for config-derived options +## 2. `options` as a config factory -- [x] 2.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` (or a new co-located spec) asserting `BaseToolConstructor.prepare()` accepts a return type of `PreparedToolOptions | void | Promise | void>` -- [x] 2.2 Define `PreparedToolOptions` in `packages/sdk/src/entities/BaseTool.ts`, initially restricted to `{ toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false }`, and update `BaseToolConstructor.prepare()`'s signature to use it -- [x] 2.3 Add a failing test in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` asserting a `BaseToolFacade` instance exposes a `preparedOptions` slot that starts `undefined` and can be set after construction, without touching `constructable.options` -- [x] 2.4 Implement the `preparedOptions` slot on `BaseToolFacade` (private field + a setter method the manager calls), keeping `constructable.options` untouched +- [x] 2.1 Add a failing test in `packages/sdk/src/entities/BaseTool.spec.ts` — `it('should accept a synchronous factory of the tool config as static options')` — asserting a `BaseToolConstructor` type-checks with `options` declared as `(config: Config) => Options` +- [x] 2.2 Add a failing test asserting the object form still type-checks unchanged — `it('should accept a plain options object as static options')` +- [x] 2.3 Define `ToolOptionsFactory` in `packages/sdk/src/entities/BaseTool.ts` and widen `BaseToolConstructor.options` to `Options | ToolOptionsFactory` +- [x] 2.4 Propagate the widened `options` type through `BlockToolConstructor`, `InlineToolConstructor`, and `BlockTuneConstructor` — no production change was needed: all three extend `BaseToolConstructor` without re-declaring `options`, so they inherit the union. Pinned by `it('should carry the factory form through to every tool subtype constructor')` +- [x] 2.5 Confirm `BaseToolConstructor.prepare()` keeps its current `void | Promise` return type — design.md Decision 3 deliberately leaves this hook untouched -## 3. Toolbox merge tiers in `BlockToolFacade` +## 3. Resolving the factory in `BaseToolFacade` -- [x] 3.1 Add failing tests in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` for `BlockToolFacade.toolbox`, one per tier combination: static-only, prepared-only, static+prepared, prepared+`use()`-override, and `use()`-time `toolbox: false` hiding the tool even when a prepared value exists -- [x] 3.2 Update `BlockToolFacade.toolbox` to seed its existing merge algorithm from `preparedOptions?.toolbox ?? constructable.options?.toolbox` instead of `constructable.options?.toolbox` alone, keeping the `use()`-time override layer unchanged on top -- [x] 3.3 Confirm existing toolbox-merge tests (static + `use()`-override only, no prepared tier) still pass unmodified — no such tests existed before this change (the `toolbox` getter had no prior spec coverage); the static-only and static+use()-override paths are now covered by 3.1's new tests instead, and the full `@editorjs/sdk` suite (31 tests) passes +- [x] 3.1 Add a failing test in `packages/sdk/src/tools/facades/BaseToolFacade.spec.ts` — `it('should call the options factory once with the config passed at use() time')` +- [x] 3.2 Add a failing test — `it('should not call the options factory again when option getters are read repeatedly')` +- [x] 3.3 Add a failing test — `it('should leave the tool class options untouched after resolving the factory')` +- [x] 3.4 Add a failing test — `it('should resolve options independently for two facades wrapping the same tool class with different configs')` (the multi-`Core` case, design.md Decision 6) +- [x] 3.5 Add a failing test — `it('should merge factory-returned config defaults with the use()-time config')` — covering that a tool instance receives the defaults the factory applied +- [x] 3.6 Implement the private resolved-options field in the `BaseToolFacade` constructor: invoke the factory with `useToolOptions.config ?? {}` when `options` is a function, otherwise use the object as-is. Declared `protected readonly` rather than `#private` because `BlockToolFacade` reads it (group 5); per-instance semantics are unchanged +- [x] 3.7 Point the `options` and `config` getters at the resolved field instead of `constructable.options` -## 4. Wiring `prepare()`'s return value through core +## 4. Reading resolved options in `BlockToolFacade` -- [x] 4.1 Add a failing test for `ToolsManager.prepareTools()` asserting that when `toolConstructor.prepare()` resolves with `{ toolbox: [...] }`, the resulting facade (as added to `available`/`unavailable` collections) has that value on `preparedOptions.toolbox` before `ToolLoadedCoreEvent` is dispatched -- [x] 4.2 Update `ToolsManager.prepareTools()` to capture `prepare()`'s resolved return value and call the facade's `preparedOptions` setter (task 2.4) before `setToAvailableToolsCollection`/`ToolLoadedCoreEvent` -- [x] 4.3 Add a failing test confirming a `prepare()` that returns `undefined`/void leaves `preparedOptions` unset (no regression for tools without config-derived options) +- [x] 4.1 Add failing tests for `BlockToolFacade.toolbox` covering both tiers against a factory-derived value — `it('should derive toolbox entries from the config passed at use() time')`, `it('should merge a use()-time toolbox override onto a factory-derived value')`, and `it('should hide the tool when a use()-time toolbox is false despite a factory-derived value')` +- [x] 4.2 Add a failing test — `it('should read isReadOnlySupported from the resolved static options')` +- [x] 4.3 Point `BlockToolFacade.toolbox` and `isReadOnlySupported` at the resolved static options, leaving the array/object positional-merge algorithm and its two tiers unchanged +- [x] 4.4 Confirm the existing object-form toolbox tests still pass unmodified — **none existed**: the toolbox merge algorithm had no test coverage at all. Added five characterization tests in the new `BlockToolFacade.spec.ts` (empty, single-entry wrapping, `false` hiding, array-onto-array positional merge, object-onto-object merge) and confirmed they pass before and after 4.3 -## 5. Dev-time unused-config-key detection +## 5. Migrating in-repo tools -- [x] 5.1 Add a failing test asserting that when a tool's resolved `ToolConfig` object has a key not passed through via `options.config` defaults and not present on a `prepare()`-returned `PreparedToolOptions`, `ToolsManager.prepareTools()` emits a `console.warn` naming the unused key, without throwing or marking the tool unavailable -- [x] 5.2 Implement the reachability check in `ToolsManager.prepareTools()` (or a small helper it calls), gated the same way existing dev diagnostics are (non-throwing, dev-only) — implemented via a `Proxy` around the resolved config passed to `prepare()`, tracking which keys are actually read -- [x] 5.3 Add a failing/passing pair of tests confirming a config key that *is* consumed (via `options.config` default or a `PreparedToolOptions`-returning `prepare()`) produces no warning +- [x] 5.1 Update `packages/tools/{paragraph,bold,italic,inline-link}` to import `ToolConfig`/their `Config` type from the new SDK location, keeping the plain-object form of `static options` — only `paragraph` referenced `ToolConfig`; `bold`, `italic`, and `inline-link` declare no config type +- [x] 5.2 Confirm `yarn workspace typecheck` and `yarn lint` pass for each of the four tools with no runtime behavior change -## 6. Spec and documentation alignment +## 6. Verification and documentation alignment -- [x] 6.1 Run `openspec validate --changes separate-tool-options-and-config --strict` (or the project's equivalent) and fix any delta-spec formatting issues — passes clean -- [x] 6.2 Update `docs/plugins.md`/`docs/architecture.md` mentions of `core.use(ToolConstructor, options)` if the described merge behavior no longer matches (cross-reference note from the proposal) — reviewed; existing mentions only describe the coarse `use()` → `initialize()` → prepare-tools → emit `ToolLoadedCoreEvent` flow, which this change preserves exactly, so no doc text is inaccurate -- [x] 6.3 Note in the PR description that this change makes the `editor-js/header#130` `HeaderConfig.levels` fix possible, and that wiring `Header.prepare()` to consume it is tracked as a follow-up in the `header` repo, not in this change — captured in `proposal.md`'s Impact section; still needs to be carried into the actual GitHub PR body when the PR is updated +- [x] 6.1 Confirm `yarn workspace @editorjs/sdk test`, `yarn workspace @editorjs/core test`, and `yarn lint` pass — 34 SDK tests, 161 core tests, lint clean +- [x] 6.2 Run `openspec validate --changes separate-tool-options-and-config --strict` and fix any delta-spec formatting issues +- [x] 6.3 Re-check `docs/plugins.md` and `docs/architecture.md` for statements about `static options` always being a plain object, and update any that no longer hold — neither file makes such a statement, no change needed +- [ ] 6.4 Update the PR description on `editor-js/document-model#188` to describe the factory form, and note that wiring `Header.options` to it is a follow-up in the `header` repo (`editor-js/header#130`), not part of this change diff --git a/packages/core/src/tools/ToolsManager.spec.ts b/packages/core/src/tools/ToolsManager.spec.ts deleted file mode 100644 index 9c51abcf..00000000 --- a/packages/core/src/tools/ToolsManager.spec.ts +++ /dev/null @@ -1,153 +0,0 @@ -/* eslint-disable jsdoc/require-jsdoc */ -import { describe, expect, it, jest } from '@jest/globals'; -import { EventBus, ToolType, CoreEventType } from '@editorjs/sdk'; -import type { - CoreConfigValidated, - ToolConstructable, - ToolLoadedCoreEvent, - ToolStaticOptions -} from '@editorjs/sdk'; -import type { EditorAPI } from '../api/index.js'; -import ToolsManager from './ToolsManager.js'; - -const createManager = (): { manager: ToolsManager; - eventBus: EventBus; } => { - const editorConfig = { - tools: {}, - defaultBlock: '__no-default-block__', - placeholder: '', - } as CoreConfigValidated; - const apiFactory = (): EditorAPI => ({} as EditorAPI); - const eventBus = new EventBus(); - const manager = new ToolsManager(editorConfig, apiFactory, eventBus); - - return { manager, - eventBus }; -}; - -describe('ToolsManager.prepareTools()', () => { - it('should capture a config-derived toolbox from prepare() on the facade before ToolLoadedCoreEvent fires', async () => { - const { manager, eventBus } = createManager(); - const toolboxFromConfig = [{ title: 'Heading 1' }]; - - class MockHeaderTool { - public static type = ToolType.Block; - public static name = 'header'; - public static prepare = jest.fn(() => Promise.resolve({ toolbox: toolboxFromConfig })); - } - - let preparedOptionsAtDispatchTime: unknown; - - eventBus.addEventListener(`core:${CoreEventType.ToolLoaded}`, (event: ToolLoadedCoreEvent) => { - preparedOptionsAtDispatchTime = event.detail.tool.preparedOptions; - }); - - await manager.prepareTools([[MockHeaderTool as unknown as ToolConstructable, undefined]]); - - expect(preparedOptionsAtDispatchTime).toEqual({ toolbox: toolboxFromConfig }); - expect(manager.available.get('header')?.preparedOptions).toEqual({ toolbox: toolboxFromConfig }); - }); - - it('should leave preparedOptions unset when prepare() resolves with undefined', async () => { - const { manager } = createManager(); - - class MockParagraphTool { - public static type = ToolType.Block; - public static name = 'paragraph'; - public static prepare = jest.fn(() => Promise.resolve(undefined)); - } - - await manager.prepareTools([[MockParagraphTool as unknown as ToolConstructable, undefined]]); - - expect(manager.available.get('paragraph')?.preparedOptions).toBeUndefined(); - }); - - it('should add the tool to available tools without calling prepare() when the tool has no prepare method', async () => { - const { manager } = createManager(); - - class MockBoldTool { - public static type = ToolType.Inline; - public static name = 'bold'; - } - - await manager.prepareTools([[MockBoldTool as unknown as ToolConstructable, undefined]]); - - expect(manager.available.get('bold')?.preparedOptions).toBeUndefined(); - }); -}); - -describe('ToolsManager.prepareTools() — unused config key detection', () => { - it('should warn about a resolved config key that prepare() never reads and that has no static default', async () => { - const { manager } = createManager(); - const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); - - class MockHeaderTool { - public static type = ToolType.Block; - public static name = 'header'; - public static prepare = jest.fn(({ config }: { config: Record }) => { - void config.defaultLevel; - - return Promise.resolve(undefined); - }); - } - - await manager.prepareTools([ - [ - MockHeaderTool as unknown as ToolConstructable, - { config: { levels: [1, 2], - defaultLevel: 1 } } as ToolStaticOptions, - ], - ]); - - expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('levels')); - - warnSpy.mockRestore(); - }); - - it('should not warn about a config key that prepare() reads', async () => { - const { manager } = createManager(); - const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); - - class MockHeaderTool { - public static type = ToolType.Block; - public static name = 'header'; - public static prepare = jest.fn(({ config }: { config: Record }) => { - void config.levels; - void config.defaultLevel; - - return Promise.resolve(undefined); - }); - } - - await manager.prepareTools([ - [ - MockHeaderTool as unknown as ToolConstructable, - { config: { levels: [1, 2], - defaultLevel: 1 } } as ToolStaticOptions, - ], - ]); - - expect(warnSpy).not.toHaveBeenCalled(); - - warnSpy.mockRestore(); - }); - - it('should not warn about a config key that is part of the tool\'s own static config defaults', async () => { - const { manager } = createManager(); - const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); - - class MockHeaderTool { - public static type = ToolType.Block; - public static name = 'header'; - // eslint-disable-next-line @typescript-eslint/no-magic-numbers -- heading levels 1-3, self-evident - public static options = { config: { levels: [1, 2, 3] } }; - public static prepare = jest.fn(() => Promise.resolve(undefined)); - } - - await manager.prepareTools([[MockHeaderTool as unknown as ToolConstructable, undefined]]); - - expect(warnSpy).not.toHaveBeenCalled(); - - warnSpy.mockRestore(); - }); -}); diff --git a/packages/core/src/tools/ToolsManager.ts b/packages/core/src/tools/ToolsManager.ts index dfa58c7f..3026fdbb 100644 --- a/packages/core/src/tools/ToolsManager.ts +++ b/packages/core/src/tools/ToolsManager.ts @@ -136,39 +136,15 @@ export default class ToolsManager { void promiseQueue.add(async () => { try { const tool = factory.get(toolName); - const resolvedConfig = tool.config; - const accessedConfigKeys = new Set(); - - /** - * Tracks which config keys `prepare()` actually reads, so a key that's set but never - * consumed anywhere reachable can be flagged below. - */ - const trackedConfig = new Proxy(resolvedConfig, { - get(target, prop, receiver) { - if (typeof prop === 'string') { - accessedConfigKeys.add(prop); - } - - return Reflect.get(target, prop, receiver); - }, - }); /** * Merged plugin `config` only (static `options().config` + `use(Tool, options).config`), aligned with `BaseToolFacade.prepare`. */ - const preparedOptions = await toolConstructor.prepare!({ + await toolConstructor.prepare!({ toolName, - config: trackedConfig, + config: tool.config, }); - /** - * Captured on the facade instance (not the shared static `options`) so config-derived - * options (e.g. `toolbox`) are available before ToolLoadedCoreEvent is dispatched below. - */ - tool.setPreparedOptions(preparedOptions ?? undefined); - - this.#warnAboutUnusedConfigKeys(toolName, toolConstructor, resolvedConfig, accessedConfigKeys); - if (tool.isInline()) { /** * Some Tools validation @@ -207,33 +183,6 @@ export default class ToolsManager { await promiseQueue.completed; } - /** - * Dev-time diagnostic: warns about a key in a tool's resolved config that has no static - * default on the tool (i.e. the tool author didn't declare it) and that `prepare()` never - * read off the config it was given. Doesn't throw and doesn't affect tool availability — - * it only helps catch a config field that's set but silently has no effect. - * @param toolName - name of the tool being prepared - * @param toolConstructor - the tool's constructable, used to read its static config defaults - * @param resolvedConfig - the tool's fully-resolved config (static defaults + `use()`-time overrides) - * @param accessedConfigKeys - keys of `resolvedConfig` that were read while `prepare()` ran - */ - #warnAboutUnusedConfigKeys( - toolName: string, - toolConstructor: ToolConstructable, - resolvedConfig: Record, - accessedConfigKeys: Set - ): void { - const staticConfigKeys = new Set(Object.keys(toolConstructor.options?.config ?? {})); - - for (const key of Object.keys(resolvedConfig)) { - if (!staticConfigKeys.has(key) && !accessedConfigKeys.has(key)) { - console.warn( - `Tool "${toolName}": config key "${key}" is set but never used (not declared as a static default and not read by prepare()). This may be a mistake.` - ); - } - } - } - /** * Unify tools config * @param config - user's tools config diff --git a/packages/sdk/src/entities/BaseTool.spec.ts b/packages/sdk/src/entities/BaseTool.spec.ts index 98909341..641e9233 100644 --- a/packages/sdk/src/entities/BaseTool.spec.ts +++ b/packages/sdk/src/entities/BaseTool.spec.ts @@ -1,68 +1,87 @@ /* eslint-disable jsdoc/require-jsdoc */ -import { describe, expect, it } from '@jest/globals'; -import type { ToolConfig, BaseToolOptions, BaseToolConstructor } from './BaseTool.js'; -interface SampleConfig { - placeholder?: string; +import { describe, expect, it } from '@jest/globals'; +import type { + BaseToolConstructor, + BaseToolOptions, + BlockToolOptions, + BlockTuneOptions, + InlineToolOptions, + ToolConfig +} from './BaseTool.js'; +import type { + BlockToolConstructor, + BlockToolData, + BlockTuneConstructor, + InlineToolConstructor +} from './index.js'; + +interface TestToolConfig { + level?: number; } -describe('ToolConfig', () => { - it('should allow a concrete shape to satisfy the SDK-owned ToolConfig contract', () => { - const config: ToolConfig = { placeholder: 'hello' }; +/** + * Resolves to `true` only when `T` is exactly `any`. + * + * `1 & any` collapses back to `any`, and `0 extends any` holds — a relation no + * other type satisfies. This is what distinguishes an SDK-owned `ToolConfig` + * from a re-export of the legacy `ToolConfig = T`. + */ +type IsAny = 0 extends (1 & T) ? true : false; - expect(config.placeholder).toBe('hello'); - }); - - it('should type an unparameterized ToolConfig as a permissive object rather than defaulting to any', () => { - const describeConfig = (config: ToolConfig): unknown => { - // @ts-expect-error -- config no longer defaults to `any`, so arbitrary property access must be narrowed first - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- the unsafe access is exactly what this test proves is now rejected - const leak: string = config.whatever; - - return leak; - }; +describe('ToolConfig', () => { + it('should expose ToolConfig from the SDK itself rather than re-export it from @editorjs/editorjs', () => { + const toolConfigIsAny: IsAny = false; + const optionsConfigIsAny: IsAny> = false; - expect(typeof describeConfig).toBe('function'); + expect(toolConfigIsAny).toBe(false); + expect(optionsConfigIsAny).toBe(false); }); - it('should type BaseToolOptions.config using the SDK-owned ToolConfig shape', () => { - const options: BaseToolOptions = { - config: { placeholder: 'hi' }, - }; + it('should keep the config option checked on every tool subtype', () => { + const blockConfigIsAny: IsAny> = false; + const inlineConfigIsAny: IsAny> = false; + const tuneConfigIsAny: IsAny> = false; - expect(options.config?.placeholder).toBe('hi'); + expect(blockConfigIsAny).toBe(false); + expect(inlineConfigIsAny).toBe(false); + expect(tuneConfigIsAny).toBe(false); }); }); -describe('prepare() return contract (PreparedToolOptions)', () => { - it('should allow prepare() to return config-derived toolbox entries', () => { - const prepare: BaseToolConstructor['prepare'] = () => { - return { toolbox: [{ title: 'Heading 1' }] }; +describe('BaseToolConstructor.options', () => { + it('should accept a synchronous factory of the tool config as static options', () => { + const tool: BaseToolConstructor = { + name: 'factory-tool', + options: config => ({ + config: { level: config.level ?? 1 }, + }), }; - expect(typeof prepare).toBe('function'); + expect(typeof tool.options).toBe('function'); }); - it('should allow prepare() to return void', () => { - const prepare: BaseToolConstructor['prepare'] = () => undefined; - - expect(typeof prepare).toBe('function'); - }); - - it('should allow prepare() to resolve a Promise of config-derived toolbox entries', () => { - const prepare: BaseToolConstructor['prepare'] = async () => { - return Promise.resolve({ toolbox: false as const }); + it('should accept a plain options object as static options', () => { + const tool: BaseToolConstructor = { + name: 'object-tool', + options: { + config: { level: 1 }, + }, }; - expect(typeof prepare).toBe('function'); + expect(typeof tool.options).toBe('object'); }); - it('should reject a prepare() return value with an unrecognized field', () => { - // @ts-expect-error -- PreparedToolOptions only recognizes `toolbox` for now - const prepare: BaseToolConstructor['prepare'] = () => { - return { notARealOption: true }; - }; - - expect(typeof prepare).toBe('function'); + it('should carry the factory form through to every tool subtype constructor', () => { + const blockOptions: BlockToolConstructor['options'] = config => ({ + toolbox: [{ title: `Level ${config.level ?? 1}`, + icon: '' }], + }); + const inlineOptions: InlineToolConstructor['options'] = () => ({ config: {} }); + const tuneOptions: BlockTuneConstructor['options'] = () => ({ config: {} }); + + expect(typeof blockOptions).toBe('function'); + expect(typeof inlineOptions).toBe('function'); + expect(typeof tuneOptions).toBe('function'); }); }); diff --git a/packages/sdk/src/entities/BaseTool.ts b/packages/sdk/src/entities/BaseTool.ts index 89773cf4..6e6a0bfd 100644 --- a/packages/sdk/src/entities/BaseTool.ts +++ b/packages/sdk/src/entities/BaseTool.ts @@ -1,13 +1,19 @@ -import type { ToolboxConfigEntry } from '@editorjs/editorjs'; import type { BlockToolOptions } from './BlockTool.js'; import type { InlineToolOptions } from './InlineTool.js'; import type { BlockTuneOptions } from './BlockTune.js'; import type { ToolType } from './EntityType.js'; /** - * Tool-author-facing user configuration, kept as a distinct SDK-owned contract from - * {@link ToolStaticOptions} (the core/plugin-facing static declaration). - * @template T - Shape of the plugin-specific configuration object. + * Plugin-specific, tool-author-facing configuration object. + * + * Kept formally distinct from a tool's static options: `options` is core/plugin-facing + * wiring read before any block instance exists, while `ToolConfig` is user data resolved + * per tool registration and handed to the tool instance. + * + * Replaces the legacy `ToolConfig` re-exported from `@editorjs/editorjs`, whose default + * type argument was `any` — meaning any tool that omitted the generic silently opted out + * of type checking on its own configuration. + * @template T - Shape of the tool's own configuration object. */ export type ToolConfig = T; @@ -34,6 +40,26 @@ export interface BaseToolOptions { [BaseToolOptionKey.Config]?: Config; } +/** + * Derives a tool's static options from its resolved configuration. + * + * Declared in place of a plain options object when some option value depends on + * config — e.g. a `toolbox` whose entries follow a `levels` config field. The + * factory is called once per tool registration, with the `config` supplied to + * `core.use(Tool, { config })`, so it must apply its own defaults for keys the + * integrator omitted. + * + * It must be synchronous: every consumer of static options reads it through a + * synchronous facade getter. Asynchronous tool initialization belongs in + * {@link BaseToolConstructor.prepare} instead. + * @template Config - Shape of the plugin-specific config object. + * @template Options - The concrete options interface for this tool type. + */ +export type ToolOptionsFactory< + Config extends ToolConfig = ToolConfig, + Options extends BaseToolOptions = BaseToolOptions +> = (config: Config) => Options; + // Re-export so consumers can import all option types from this file export type { BlockToolOptions, InlineToolOptions, BlockTuneOptions }; @@ -63,18 +89,6 @@ export type ToolTypeToOptions = { [ToolType.Tune]: BlockTuneOptions; }; -/** - * Static options a tool can compute from its own resolved {@link ToolConfig}, as an override for - * the fixed defaults declared on {@link BaseToolConstructor.options}. - */ -export interface PreparedToolOptions { - /** - * Toolbox entry (or entries) computed from the tool's resolved config. Set to `false` to hide - * the tool from the toolbox. - */ - toolbox?: ToolboxConfigEntry | ToolboxConfigEntry[] | false; -} - /** * Common interface for Tool constructor (static) side. * @template Config - Shape of the plugin-specific config object. Passed to @@ -96,21 +110,21 @@ export interface BaseToolConstructor< * All static configuration for the tool. * Values here are defaults; they can be overridden via the second argument * of `core.use(Tool, options)`. + * + * May also be a {@link ToolOptionsFactory} — a synchronous function of the tool's + * config — when option values are derived from configuration. The facade resolves + * it once, per registration, and never writes the result back onto the tool class. */ - options?: Options; + options?: Options | ToolOptionsFactory; /** * Tool's prepare method. Can be async. - * May return config-derived static options (currently just `toolbox`), computed from the - * fully-resolved `config`. Implementations never need to mutate this constructor's shared - * static `options` to have an effect — see {@link PreparedToolOptions}. * @param data - Object with toolName and config properties * @param data.toolName - Tool's own name * @param data.config - Merged plugin configuration */ // eslint-disable-next-line -- ESLint doesn't understand it's a type - prepare?(data: { toolName: string, config: Config }): - PreparedToolOptions | void | Promise; + prepare?(data: { toolName: string, config: Config }): void | Promise; /** * Tool's reset method to clean up anything set by prepare. Can be async. diff --git a/packages/sdk/src/tools/facades/BaseToolFacade.spec.ts b/packages/sdk/src/tools/facades/BaseToolFacade.spec.ts index 4164814e..73a6b892 100644 --- a/packages/sdk/src/tools/facades/BaseToolFacade.spec.ts +++ b/packages/sdk/src/tools/facades/BaseToolFacade.spec.ts @@ -12,6 +12,31 @@ import type { EditorAPI } from '../../api'; const emptyApi = {} as EditorAPI; +/** + * Wraps an already-declared tool class in a facade, so a test can keep a reference + * to the class itself (e.g. to assert its static `options` were not mutated). + * @param constructable - tool class to wrap + * @param useToolOptions - second argument of `use(Tool, options)` + * @param facadeOpts - `isDefault` / `defaultPlaceholder` for the facade constructor + */ +function facadeFor( + constructable: unknown, + useToolOptions: ToolOptions, + facadeOpts: { + isDefault?: boolean; + defaultPlaceholder?: string | false; + } = {} +): BlockToolFacade { + return new BlockToolFacade({ + api: emptyApi, + constructable: constructable as BlockToolConstructor, + defaultPlaceholder: facadeOpts.defaultPlaceholder, + isDefault: facadeOpts.isDefault ?? false, + name: 'test-tool', + useToolOptions, + }); +} + /** * Block tool facade with only fields needed to exercise BaseToolFacade getters. * @param staticOptions - optional object set on the mock class as static `options` @@ -19,7 +44,7 @@ const emptyApi = {} as EditorAPI; * @param facadeOpts - `isDefault` / `defaultPlaceholder` for the facade constructor */ function createBlockFacade( - staticOptions: Record | undefined, + staticOptions: unknown, useToolOptions: ToolOptions, facadeOpts: { isDefault?: boolean; @@ -38,14 +63,12 @@ function createBlockFacade( }); } - return new BlockToolFacade({ - api: emptyApi, - constructable: MockBlockTool as unknown as BlockToolConstructor, - defaultPlaceholder: facadeOpts.defaultPlaceholder, - isDefault: facadeOpts.isDefault ?? false, - name: 'test-tool', - useToolOptions, - }); + return facadeFor(MockBlockTool, useToolOptions, facadeOpts); +} + +interface LevelsConfig { + levels?: number[]; + defaultLevel?: number; } describe('BaseToolFacade (via BlockToolFacade)', () => { @@ -164,77 +187,100 @@ describe('BaseToolFacade (via BlockToolFacade)', () => { }); }); - describe('preparedOptions', () => { - it('should start undefined before setPreparedOptions is called', () => { - const facade = createBlockFacade({}, {} as ToolOptions); - - expect(facade.preparedOptions).toBeUndefined(); - }); + describe('options factory', () => { + it('should call the options factory once with the config passed at use() time', () => { + const seen: LevelsConfig[] = []; + const factory = (config: LevelsConfig): Record => { + seen.push(config); - it('should expose the value passed to setPreparedOptions', () => { - const facade = createBlockFacade({}, {} as ToolOptions); + return { config }; + }; - facade.setPreparedOptions({ toolbox: [{ title: 'Heading 1' }] }); + createBlockFacade(factory, { + [UserToolOptions.Config]: { levels: [1, 3] }, + } as ToolOptions); - expect(facade.preparedOptions).toEqual({ toolbox: [{ title: 'Heading 1' }] }); + expect(seen).toEqual([{ levels: [1, 3] }]); }); - it('should not mutate the tool constructor\'s static options when set', () => { - const staticOptions = { toolbox: { title: 'Static' } }; - const facade = createBlockFacade(staticOptions, {} as ToolOptions); + it('should not call the options factory again when option getters are read repeatedly', () => { + let callCount = 0; + const factory = (): Record => { + callCount += 1; - facade.setPreparedOptions({ toolbox: [{ title: 'Computed' }] }); + return { config: { levels: [1] } }; + }; - expect(staticOptions).toEqual({ toolbox: { title: 'Static' } }); - }); - }); + const facade = createBlockFacade(factory, {} as ToolOptions); - describe('toolbox getter (merge tiers)', () => { - it('should return the static toolbox entry when there is no prepared or use()-time value', () => { - const facade = createBlockFacade( - { toolbox: { title: 'Static' } }, - {} as ToolOptions - ); + void facade.options; + void facade.config; + void facade.options; - expect(facade.toolbox).toEqual([{ title: 'Static' }]); + expect(callCount).toBe(1); }); - it('should return the prepared toolbox entries when the tool has no static toolbox', () => { - const facade = createBlockFacade(undefined, {} as ToolOptions); + it('should leave the tool class options untouched after resolving the factory', () => { + const factory = (config: LevelsConfig): Record => ({ + config: { levels: config.levels ?? [1] }, + }); + + class ToolWithFactoryOptions { + public static type = ToolType.Block; + public static options = factory; + } - facade.setPreparedOptions({ toolbox: [{ title: 'Heading 1' }, { title: 'Heading 2' }] }); + facadeFor(ToolWithFactoryOptions, { + [UserToolOptions.Config]: { levels: [2] }, + } as ToolOptions); - expect(facade.toolbox).toEqual([{ title: 'Heading 1' }, { title: 'Heading 2' }]); + expect(ToolWithFactoryOptions.options).toBe(factory); }); - it('should layer an explicit use()-time override on top of the prepared toolbox', () => { - const facade = createBlockFacade( - undefined, - { toolbox: [{ title: 'Heading 1 override' }] } as ToolOptions - ); + it('should resolve options independently for two facades wrapping the same tool class with different configs', () => { + class SharedTool { + public static type = ToolType.Block; + public static options = (config: LevelsConfig): Record => { + const levels = config.levels ?? [1]; + + return { config: { defaultLevel: levels[levels.length - 1] } }; + }; + } - facade.setPreparedOptions({ toolbox: [{ title: 'Heading 1' }, { title: 'Heading 2' }] }); + const first = facadeFor(SharedTool, { + [UserToolOptions.Config]: { levels: [1] }, + } as ToolOptions); + const second = facadeFor(SharedTool, { + [UserToolOptions.Config]: { levels: [2, 3] }, + } as ToolOptions); - expect(facade.toolbox).toEqual([{ title: 'Heading 1 override' }]); + expect(first.config).toEqual({ + levels: [1], + defaultLevel: 1, + }); + expect(second.config).toEqual({ + levels: [2, 3], + defaultLevel: 3, + }); }); - it('should hide the tool from the toolbox when use()-time toolbox is false, even with a prepared value', () => { + it('should merge factory-returned config defaults with the use()-time config', () => { const facade = createBlockFacade( - undefined, - { toolbox: false } as ToolOptions + (config: LevelsConfig): Record => ({ + config: { + levels: config.levels ?? [1, 2, 3], + defaultLevel: config.defaultLevel ?? 2, + }, + }), + { + [UserToolOptions.Config]: { levels: [4] }, + } as ToolOptions ); - facade.setPreparedOptions({ toolbox: [{ title: 'Heading 1' }] }); - - expect(facade.toolbox).toBeUndefined(); - }); - - it('should hide the tool from the toolbox when the prepared value is false and there is no use()-time override', () => { - const facade = createBlockFacade(undefined, {} as ToolOptions); - - facade.setPreparedOptions({ toolbox: false }); - - expect(facade.toolbox).toBeUndefined(); + expect(facade.config).toEqual({ + levels: [4], + defaultLevel: 2, + }); }); }); diff --git a/packages/sdk/src/tools/facades/BaseToolFacade.ts b/packages/sdk/src/tools/facades/BaseToolFacade.ts index e0752701..77697b23 100644 --- a/packages/sdk/src/tools/facades/BaseToolFacade.ts +++ b/packages/sdk/src/tools/facades/BaseToolFacade.ts @@ -9,8 +9,8 @@ import { type BlockTuneFacade } from './BlockTuneFacade.js'; import type { BlockTool, BlockToolConstructor, InlineTool, InlineToolConstructor, BlockTuneConstructor, ToolTypeToOptions, ToolStaticOptions, BlockToolOptions, InlineToolOptions, BlockTuneOptions, - PreparedToolOptions -} from '../../entities'; + ToolOptionsFactory +} from '../../entities/index.js'; import type { EditorAPI } from '../../api'; // eslint-disable-next-line @typescript-eslint/no-explicit-any -- need to allow any type here so extended interfaces pass @@ -41,6 +41,28 @@ export type ToolOptions = ToolStaticOptions; // Re-export per-tool option types so consumers can import them from here export type { BlockToolOptions, InlineToolOptions, BlockTuneOptions }; +/** + * Resolves a tool class's static options for a single registration. + * + * A tool may declare `options` either as a plain object or as a {@link ToolOptionsFactory} + * of its config. The factory form is invoked here — once, synchronously — with the `config` + * supplied to `use(Tool, { config })`, and applies its own defaults for keys the integrator + * omitted. + * @param constructable - tool class being registered + * @param useToolOptions - second argument of `use(Tool, options)` + */ +function resolveStaticOptions(constructable: ToolConstructable, useToolOptions: ToolOptions): ToolOptions { + const staticOptions = constructable.options; + + if (isFunction(staticOptions)) { + const factory = staticOptions as ToolOptionsFactory; + + return factory(useToolOptions[UserToolOptions.Config] ?? {}) as ToolOptions; + } + + return staticOptions ?? {}; +} + /** * BlockToolFacade constructor options inteface */ @@ -106,16 +128,19 @@ export abstract class BaseToolFacade { - const staticOpts = this.constructable.options; - const fromTool = (staticOpts?.[BaseToolOptionKey.Config] ?? {}) as Record; + const fromTool = (this.resolvedStaticOptions[BaseToolOptionKey.Config] ?? {}) as Record; const fromUse = (this.useToolOptions[UserToolOptions.Config] ?? {}) as Record; const merged: Record = { ...fromTool, @@ -189,7 +212,7 @@ export abstract class BaseToolFacade { + public prepare(): void | Promise { // eslint-disable-next-line @typescript-eslint/unbound-method if (isFunction(this.constructable.prepare)) { return this.constructable.prepare({ @@ -199,23 +222,6 @@ export abstract class BaseToolFacade { + describe('toolbox getter with a plain options object', () => { + it('should return undefined when the tool declares no toolbox', () => { + const facade = createFacade({}); + + expect(facade.toolbox).toBeUndefined(); + }); + + it('should wrap a single toolbox entry into an array', () => { + const facade = createFacade({ + [BlockToolOptionKey.Toolbox]: { + title: 'Text', + icon: '', + }, + }); + + expect(facade.toolbox).toEqual([{ + title: 'Text', + icon: '', + }]); + }); + + it('should hide the tool when the use()-time toolbox is false', () => { + const facade = createFacade( + { [BlockToolOptionKey.Toolbox]: { title: 'Text' } }, + { [UserToolOptions.Toolbox]: false } as ToolOptions + ); + + expect(facade.toolbox).toBeUndefined(); + }); + + it('should merge a use()-time toolbox array onto the tool array positionally', () => { + const facade = createFacade( + { + [BlockToolOptionKey.Toolbox]: [ + { + title: 'H1', + icon: 'tool-1', + }, + { + title: 'H2', + icon: 'tool-2', + }, + ], + }, + { + [UserToolOptions.Toolbox]: [{ title: 'Heading 1' }], + } as ToolOptions + ); + + expect(facade.toolbox).toEqual([{ + title: 'Heading 1', + icon: 'tool-1', + }]); + }); + + it('should merge a use()-time toolbox object onto a tool toolbox object', () => { + const facade = createFacade( + { + [BlockToolOptionKey.Toolbox]: { + title: 'Text', + icon: 'tool-icon', + }, + }, + { + [UserToolOptions.Toolbox]: { title: 'Paragraph' }, + } as ToolOptions + ); + + expect(facade.toolbox).toEqual([{ + title: 'Paragraph', + icon: 'tool-icon', + }]); + }); + }); + + describe('toolbox getter with an options factory', () => { + it('should derive toolbox entries from the config passed at use() time', () => { + const facade = createFacade( + (config: LevelsConfig): Record => ({ + [BlockToolOptionKey.Toolbox]: (config.levels ?? [1, 2, 3]).map(level => ({ + title: `Heading ${level}`, + data: { level }, + })), + }), + { [UserToolOptions.Config]: { levels: [2, 4] } } as ToolOptions + ); + + expect(facade.toolbox).toEqual([ + { + title: 'Heading 2', + data: { level: 2 }, + }, + { + title: 'Heading 4', + data: { level: 4 }, + }, + ]); + }); + + it('should merge a use()-time toolbox override onto a factory-derived value', () => { + const facade = createFacade( + (config: LevelsConfig): Record => ({ + [BlockToolOptionKey.Toolbox]: (config.levels ?? [1]).map(level => ({ + title: `Heading ${level}`, + icon: `icon-${level}`, + })), + }), + { + [UserToolOptions.Config]: { levels: [1, 2] }, + [UserToolOptions.Toolbox]: [{ title: 'Title' }], + } as ToolOptions + ); + + expect(facade.toolbox).toEqual([{ + title: 'Title', + icon: 'icon-1', + }]); + }); + + it('should hide the tool when a use()-time toolbox is false despite a factory-derived value', () => { + const facade = createFacade( + (config: LevelsConfig): Record => ({ + [BlockToolOptionKey.Toolbox]: (config.levels ?? [1]).map(level => ({ title: `Heading ${level}` })), + }), + { + [UserToolOptions.Config]: { levels: [1, 2] }, + [UserToolOptions.Toolbox]: false, + } as ToolOptions + ); + + expect(facade.toolbox).toBeUndefined(); + }); + }); + + describe('isReadOnlySupported getter', () => { + it('should read isReadOnlySupported from a plain options object', () => { + const facade = createFacade({ [BlockToolOptionKey.IsReadOnlySupported]: true }); + + expect(facade.isReadOnlySupported).toBe(true); + }); + + it('should read isReadOnlySupported from the resolved static options', () => { + const facade = createFacade( + (config: LevelsConfig): Record => ({ + [BlockToolOptionKey.IsReadOnlySupported]: (config.levels ?? []).length > 0, + }), + { [UserToolOptions.Config]: { levels: [1] } } as ToolOptions + ); + + expect(facade.isReadOnlySupported).toBe(true); + }); + }); +}); diff --git a/packages/sdk/src/tools/facades/BlockToolFacade.ts b/packages/sdk/src/tools/facades/BlockToolFacade.ts index 1a153792..3d26cca0 100644 --- a/packages/sdk/src/tools/facades/BlockToolFacade.ts +++ b/packages/sdk/src/tools/facades/BlockToolFacade.ts @@ -51,6 +51,12 @@ export class BlockToolFacade extends BaseToolFacade { */ protected declare useToolOptions: BlockToolOptions; + /** + * Narrowed to BlockToolOptions — the tool's static options after the facade resolved + * them, whether they were declared as a plain object or as a factory of the config + */ + protected declare readonly resolvedStaticOptions: BlockToolOptions; + /** * Creates new Tool instance * @param options - Tool constructor options @@ -73,7 +79,7 @@ export class BlockToolFacade extends BaseToolFacade { * Returns true if read-only mode is supported by Tool */ public get isReadOnlySupported(): boolean { - return this.constructable.options?.[BlockToolOptionKey.IsReadOnlySupported] === true; + return this.resolvedStaticOptions[BlockToolOptionKey.IsReadOnlySupported] === true; } /** @@ -85,49 +91,33 @@ export class BlockToolFacade extends BaseToolFacade { // } /** - * Returns Tool toolbox configuration (internal, config-derived, or user-specified). - * - * Resolved in three tiers, each layered on top of the previous: + * Returns Tool toolbox configuration (internal or user-specified). * - * 1. The static default (`constructable.options.toolbox`). - * 2. The config-derived value computed by the tool's `prepare()`, if any - * ({@link BaseToolFacade.preparedOptions}) — when present, it replaces the static default outright. - * 3. An explicit `use(Tool, options)` override, merged into whichever of the above applies - * using the rules below. + * Merges internal and user-defined toolbox configs based on the following rules: * - * Merging the resolved tool-side settings with a `use()` override follows these rules: - * - * - If both are arrays their items are merged. Length of the second one is kept. + * - If both internal and user-defined toolbox configs are arrays their items are merged. + * Length of the second one is kept. * * - If both are objects their properties are merged. * - * - If one is an object and another is an array than tool-side config is replaced with user-defined + * - If one is an object and another is an array than internal config is replaced with user-defined * config. This is made to allow user to override default tool's toolbox representation (single/multiple entries) - * - * `false` (from either the tool side or an explicit `use()` override) hides the tool from the - * toolbox entirely, unless a `use()` override supplies a real value on top of it. */ public get toolbox(): ToolboxConfigEntry[] | undefined { - const toolToolboxSettings = ( - this.preparedOptions?.[BlockToolOptionKey.Toolbox] - ?? this.constructable.options?.[BlockToolOptionKey.Toolbox] - ) as ToolboxConfig | false | undefined; + const toolToolboxSettings = this.resolvedStaticOptions[BlockToolOptionKey.Toolbox] as ToolboxConfig; const userToolboxSettings = this.useToolOptions[UserToolOptions.Toolbox]; - if (userToolboxSettings === false) { + if (isEmpty(toolToolboxSettings)) { return; } - if (!userToolboxSettings && (toolToolboxSettings === false || isEmpty(toolToolboxSettings))) { + if (userToolboxSettings === false) { return; } /** * Return tool's toolbox settings if user settings are not defined */ if (!userToolboxSettings) { - /** - * `toolToolboxSettings` can't be `false`/empty here — that was already returned above - */ - return Array.isArray(toolToolboxSettings) ? toolToolboxSettings : [toolToolboxSettings as ToolboxConfigEntry]; + return Array.isArray(toolToolboxSettings) ? toolToolboxSettings : [toolToolboxSettings]; } /** From ad736a5163d8afaf99c612fc0cdee197b75b71a1 Mon Sep 17 00:00:00 2001 From: Reversean Date: Tue, 4 Aug 2026 23:40:53 +0300 Subject: [PATCH 4/6] docs(openspec): drop tool-specific framing from the change artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit State the motivation structurally — a config field cannot reach the static option it is meant to drive — instead of anchoring it to one tool's bug. Co-Authored-By: Claude Opus 5 --- .../design.md | 21 +++++++++---------- .../proposal.md | 3 +-- .../separate-tool-options-and-config/tasks.md | 1 - 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/openspec/changes/separate-tool-options-and-config/design.md b/openspec/changes/separate-tool-options-and-config/design.md index 65ead24f..24d25077 100644 --- a/openspec/changes/separate-tool-options-and-config/design.md +++ b/openspec/changes/separate-tool-options-and-config/design.md @@ -7,10 +7,10 @@ Today a tool's static surface is one bag (`ToolConstructor.options`) that mixes `ToolConfig` itself is not an SDK contract — it's re-exported from the legacy `@editorjs/editorjs` package as `type ToolConfig = T`, an untyped passthrough. A tool author gets whatever structure they declare in their own `Config` generic parameter, with nothing checking that it's consistent with anything else. -The concrete failure this produces is the Header tool's v3 migration (draft PR [editor-js/header#130](https://github.com/editor-js/header/pull/130)): `HeaderConfig.levels` is declared but never read anywhere — the static `options.toolbox` array is a hardcoded 3-entry list (H1–H3) that has no way to depend on `config.levels`, because `options` is evaluated once, statically, at class-definition time, while `config` is only resolved per tool-registration (in `BaseToolFacade.config`) and per block instance (in the constructor). +The concrete failure this produces: a tool declares a config field intended to shape one of its static options — a set of variants that should become toolbox entries, a mode that should pick a shortcut — and the field is inert. `options` is evaluated once, statically, at class-definition time, while `config` is only resolved per tool-registration (in `BaseToolFacade.config`) and per block instance (in the constructor), so no option value can be a function of it. The field remains in the tool's `Config` type and in its documentation while the option keeps its hardcoded value. Constraints this design has to respect: -- A page can run multiple `Core` instances that both `use()` the same tool class (e.g. two editors sharing one `Header` import). Anything a tool computes from its own resolved config must not be written onto the shared class/static object, or the second instance's config would clobber the first's. +- A page can run multiple `Core` instances that both `use()` the same tool class (e.g. two editors sharing one imported class). Anything a tool computes from its own resolved config must not be written onto the shared class/static object, or the second instance's config would clobber the first's. - Every consumer of a tool's static options reads it through a **synchronous** getter (`BlockToolFacade.toolbox`, `isReadOnlySupported`, `conversionConfig`, `InlineToolFacade`'s option reads). Whatever produces those values must therefore be resolvable synchronously by the time the facade exists. - Per this change's proposal, a breaking change to `ToolConfig`'s type/import and to the `static options` type is acceptable; a live "change config after mount" API is explicitly out of scope. - The project's existing TDD convention applies to the facade changes below (see `openspec/config.yaml` rules). @@ -28,7 +28,7 @@ Constraints this design has to respect: - No change to `prepare()`. Async, side-effectful tool initialization keeps working exactly as it does today. - No change to how `toolbox`, `shortcut`, `inlineToolbar`, or `tunes` are merged with `use()`-time overrides — the merge algorithm and its tiers stay as they are. - No dev-time "declared but unused config key" diagnostic in this change (see Decision 8). -- Not migrating the `header` tool itself — it lives in a separate repo/submodule. This change ships the SDK mechanism; wiring `Header.options` to use it is a follow-up in that repo. +- Not migrating tools that live in their own repositories. This change ships the SDK mechanism; adopting the factory form is each tool's own follow-up. ## Decisions @@ -54,12 +54,12 @@ The two hooks answer different questions and have different natures. `prepare()` **4. The factory receives the `use()`-time config, and applies its own defaults inside its body.** The argument is `useToolOptions.config ?? {}` — exactly the object an integrator passed to `core.use(Tool, { config })`. A tool applies its defaults where it derives from them: ```ts -static options = (config: HeaderConfig) => { - const levels = config.levels ?? [1, 2, 3]; +static options = (config: MyToolConfig) => { + const variants = config.variants ?? DEFAULT_VARIANTS; return { - config: { levels, defaultLevel: config.defaultLevel ?? 2 }, - toolbox: levels.map(level => ({ title: `Heading ${level}`, icon: ICONS[level], data: { level } })), + config: { variants, defaultVariant: config.defaultVariant ?? variants[0] }, + toolbox: variants.map(variant => ({ title: titleFor(variant), icon: ICONS[variant], data: { variant } })), }; }; ``` @@ -69,11 +69,11 @@ static options = (config: HeaderConfig) => { **5. The factory is synchronous.** Every consumer of static options is a synchronous getter, so allowing a `Promise` return would reintroduce precisely the eager-resolve-and-cache problem Decision 3 removes: `ToolsManager` would again have to await the value and inject it before `ToolLoadedCoreEvent`, and the facade would again need a "not resolved yet" state. A tool that needs asynchronous work before it can operate still has `prepare()`; that work simply cannot feed the tool's static declaration. -*Alternative considered*: `options?: Options | ((config) => Options | Promise)`. Rejected — no evidenced case (nothing in-repo, and the motivating Header case is a pure array map), and it costs the entire simplification. +*Alternative considered*: `options?: Options | ((config) => Options | Promise)`. Rejected — no evidenced case (nothing in-repo, and the derivations this targets are pure maps over config values), and it costs the entire simplification. **6. The factory is resolved once, in the facade constructor, into a private per-instance field.** `BaseToolFacade` computes `isFunction(constructable.options) ? constructable.options(useConfig) : (constructable.options ?? {})` at construction time and keeps the result in a private field. Every option-reading getter — `options` and `config` on `BaseToolFacade`, `toolbox` and `isReadOnlySupported` on `BlockToolFacade` — reads that field rather than `constructable.options`. Those four getters are the only places in the repo that read a tool class's static options directly, so the conversion is fully enumerable. -This makes the multi-`Core` constraint hold *by construction* rather than by mitigation: the factory is a pure function on the class, each facade calls it with its own `useToolOptions.config`, and nothing is ever written back to the shared `constructable`. Two `Core` instances using one `Header` import with different `levels` cannot interfere. +This makes the multi-`Core` constraint hold *by construction* rather than by mitigation: the factory is a pure function on the class, each facade calls it with its own `useToolOptions.config`, and nothing is ever written back to the shared `constructable`. Two `Core` instances using one imported tool class with different configs cannot interfere. *Alternative considered*: resolve lazily and memoize on first read. Rejected as equivalent in effect but worse in failure mode — a throwing factory would surface at an arbitrary getter read rather than at tool registration. *Alternative considered*: call the factory on every getter read. Rejected — a tool author would reasonably assume a single call, and repeated calls would make `toolbox` entry identity unstable across reads. @@ -82,7 +82,7 @@ This makes the multi-`Core` constraint hold *by construction* rather than by mit *Alternative considered*: a third tier, in which a separately-derived toolbox value takes precedence over the static one before the `use()` override is applied on top (`derived ?? static`, then `use()`). Rejected with Decision 3 — a third tier is only needed when derivation arrives through a channel *alongside* `options`; when the factory *is* `options`, there is nothing left to layer. **8. Detecting a declared-but-unused config key is out of scope for this change.** -A tempting companion feature is a dev-time warning when a tool declares a `ToolConfig` key that nothing ever reads — the `levels` drift would then announce itself instead of failing silently. The obvious implementation, wrapping the resolved config in a `Proxy` that records key access during tool registration, does not survive contact with the lifecycle: the only registration-time hook a tool exposes is `prepare()`, which most tools — including all four in-repo ones — do not define at all, so the check would simply never run for them; and a key read in the tool's *constructor*, which is exactly where Header reads `defaultLevel`, happens per block instance, after any registration-time observation window has closed, so it would be reported as unused. Getting this right needs a reachability model — a decision about what "used" means — that this change does not otherwise require, and the motivating bug is fixed by making `levels` *derivable*, not by warning about it. +A tempting companion feature is a dev-time warning when a tool declares a `ToolConfig` key that nothing ever reads — an inert field would then announce itself instead of failing silently. The obvious implementation, wrapping the resolved config in a `Proxy` that records key access during tool registration, does not survive contact with the lifecycle: the only registration-time hook a tool exposes is `prepare()`, which most tools — including all four in-repo ones — do not define at all, so the check would never run for them; and a key read in the tool's *constructor* happens per block instance, after any registration-time observation window has closed, so it would be reported as unused. Getting this right needs a reachability model — a decision about what "used" means — that this change does not otherwise require, and the bug is fixed by making the field *derivable*, not by warning about it. *Alternative considered*: keep it and fix it in place (run for all tools, treat any config read as use). Rejected as scope creep onto a contract change — it needs its own design discussion about the reachability model. **9. Tools that keep the object form of `options` are unaffected at runtime.** @@ -93,7 +93,6 @@ A tempting companion feature is a dev-time warning when a tool declares a `ToolC - **[Risk]** `options` becoming a union (object or factory) means any code reading it must narrow first, and a tool's static options can no longer be inspected without invoking the factory. → **Mitigation**: resolution is centralized in one place (the `BaseToolFacade` constructor) and the 5 existing read sites are converted to read the resolved field; the facade is the sanctioned read path for everything outside the tool itself, and there is no in-repo consumer that reads `Tool.options` directly off a class. - **[Risk]** For factory-form tools the framework no longer merges `options.config` defaults into the value handed to the derivation, so a tool author who forgets a `?? default` gets `undefined` where the object form would have given them a default. → **Mitigation**: the factory's `config` parameter is typed as the tool's own `Config`, so the optionality is visible at the call site; and the defaults the factory returns under `config` still flow to the tool instance through the unchanged `BaseToolFacade.config` merge. - **[Risk]** Sync-only factories mean a genuinely async-derived option (e.g. a toolbox built from a fetched preset list) is not expressible. → **Mitigation**: no such case exists in-repo or in the motivating PR; if one appears, revisiting Decision 5 is a contained change (the facade would need a resolved/unresolved state and `ToolsManager` an await point) rather than a redesign. -- **[Risk]** The validating real-world case (`editor-js/header#130`) lives outside this repo and could merge with the bug still present before this change ships its mechanism. → **Mitigation**: tasks.md treats that PR as an acceptance reference, not a task owned by this change; wiring `Header.options` to the new form is explicitly a follow-up in the `header` repo. ## Migration Plan diff --git a/openspec/changes/separate-tool-options-and-config/proposal.md b/openspec/changes/separate-tool-options-and-config/proposal.md index 5c76b212..5c4278ef 100644 --- a/openspec/changes/separate-tool-options-and-config/proposal.md +++ b/openspec/changes/separate-tool-options-and-config/proposal.md @@ -2,7 +2,7 @@ Tools expose two static surfaces with no formal separation: `static options` (core/plugin-facing wiring — `toolbox`, `shortcut`, `inlineToolbar`, `tunes`, `conversionConfig`, `canBeSplit`) and `ToolConfig` (plugin-specific user data, typed only via a generic `Config extends ToolConfig = any` re-exported from the legacy `@editorjs/editorjs` package, where `ToolConfig = T` is an untyped passthrough). Because `options` is a plain static value evaluated once at class-definition time, a tool has no way to express "these option values are derived from my resolved config" — there is no contract connecting a `ToolConfig` field to the `options` it is meant to drive. -This is not hypothetical: it is live today in the Header tool's v3 SDK migration (draft PR [editor-js/header#130](https://github.com/editor-js/header/pull/130)). `HeaderConfig` declares `levels` and `defaultLevel`; `defaultLevel` flows through (it is read inside the constructor from the resolved `config` object), but `levels` is dead — the static `options.toolbox` array is hardcoded to exactly three entries (H1–H3) and never consults `config.levels`, so a user who configures `levels: [1]` sees no change in the toolbox. +The gap is structural rather than stylistic. `options` is read off the class before any registration exists, while `config` is resolved per tool registration (in `BaseToolFacade.config`) and per block instance (in the tool's constructor). A config field meant to drive an option value — toolbox entries following a set of variants, a shortcut depending on a mode, conversion behavior gated by a flag — has no path to it. The field stays declared in the tool's `Config` type and documented for integrators, while the option it was meant to control keeps whatever the class hardcoded, and nothing reports the mismatch. ## What Changes @@ -29,5 +29,4 @@ This is not hypothetical: it is live today in the Header tool's v3 SDK migration - `packages/sdk/src/tools/facades/BlockToolFacade.ts`: the `toolbox` and `isReadOnlySupported` getters read the resolved static options. The `toolbox` merge algorithm and its two tiers (tool-side value, then `use()`-time override) are unchanged. - `packages/core/src/tools/ToolsManager.ts`: unchanged. Resolution happens entirely inside the SDK facade, so the tool-registration lifecycle gains no new step and no new ordering guarantee. - `packages/tools/{paragraph,bold,italic,inline-link}`: migrate to the new `ToolConfig` import/contract (type-only change; all keep object-form `options`). -- `editor-js/header` (external submodule repo, PR #130): not modified by this change directly, but is the motivating and validating case — its `HeaderConfig.levels` drift is the concrete bug this change makes fixable. - `openspec/specs/sdk/spec.md`: delta spec updates to the "Tool and tune contracts" requirement. diff --git a/openspec/changes/separate-tool-options-and-config/tasks.md b/openspec/changes/separate-tool-options-and-config/tasks.md index 1969d182..c0d91979 100644 --- a/openspec/changes/separate-tool-options-and-config/tasks.md +++ b/openspec/changes/separate-tool-options-and-config/tasks.md @@ -39,4 +39,3 @@ - [x] 6.1 Confirm `yarn workspace @editorjs/sdk test`, `yarn workspace @editorjs/core test`, and `yarn lint` pass — 34 SDK tests, 161 core tests, lint clean - [x] 6.2 Run `openspec validate --changes separate-tool-options-and-config --strict` and fix any delta-spec formatting issues - [x] 6.3 Re-check `docs/plugins.md` and `docs/architecture.md` for statements about `static options` always being a plain object, and update any that no longer hold — neither file makes such a statement, no change needed -- [ ] 6.4 Update the PR description on `editor-js/document-model#188` to describe the factory form, and note that wiring `Header.options` to it is a follow-up in the `header` repo (`editor-js/header#130`), not part of this change From 47c9478cec03f47d4ca5ababfed42e6656e80f36 Mon Sep 17 00:00:00 2001 From: Reversean Date: Wed, 5 Aug 2026 18:30:50 +0300 Subject: [PATCH 5/6] fix: moved resolveStaticOptions to BlockToolFacade private methods --- .../sdk/src/tools/facades/BaseToolFacade.ts | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/packages/sdk/src/tools/facades/BaseToolFacade.ts b/packages/sdk/src/tools/facades/BaseToolFacade.ts index 77697b23..653e0ba1 100644 --- a/packages/sdk/src/tools/facades/BaseToolFacade.ts +++ b/packages/sdk/src/tools/facades/BaseToolFacade.ts @@ -41,28 +41,6 @@ export type ToolOptions = ToolStaticOptions; // Re-export per-tool option types so consumers can import them from here export type { BlockToolOptions, InlineToolOptions, BlockTuneOptions }; -/** - * Resolves a tool class's static options for a single registration. - * - * A tool may declare `options` either as a plain object or as a {@link ToolOptionsFactory} - * of its config. The factory form is invoked here — once, synchronously — with the `config` - * supplied to `use(Tool, { config })`, and applies its own defaults for keys the integrator - * omitted. - * @param constructable - tool class being registered - * @param useToolOptions - second argument of `use(Tool, options)` - */ -function resolveStaticOptions(constructable: ToolConstructable, useToolOptions: ToolOptions): ToolOptions { - const staticOptions = constructable.options; - - if (isFunction(staticOptions)) { - const factory = staticOptions as ToolOptionsFactory; - - return factory(useToolOptions[UserToolOptions.Config] ?? {}) as ToolOptions; - } - - return staticOptions ?? {}; -} - /** * BlockToolFacade constructor options inteface */ @@ -165,7 +143,7 @@ export abstract class BaseToolFacade Date: Wed, 5 Aug 2026 20:50:09 +0300 Subject: [PATCH 6/6] fix: handle potential null options from factory Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/sdk/src/tools/facades/BaseToolFacade.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/sdk/src/tools/facades/BaseToolFacade.ts b/packages/sdk/src/tools/facades/BaseToolFacade.ts index 653e0ba1..96a16180 100644 --- a/packages/sdk/src/tools/facades/BaseToolFacade.ts +++ b/packages/sdk/src/tools/facades/BaseToolFacade.ts @@ -244,7 +244,7 @@ export abstract class BaseToolFacade