feat(sdk): derive static tool options from tool config - #188
Open
Reversean wants to merge 6 commits into
Open
Conversation
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 <noreply@anthropic.com>
…)-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 <noreply@anthropic.com>
Reversean
changed the title
Reversean
marked this pull request as ready for review
July 29, 2026 15:09
Unit Tests
Mutation Tests
|
gohabereg
reviewed
Jul 29, 2026
| **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 }): |
Member
There was a problem hiding this comment.
I think going through prepare is an extra step. We can extends options property to be method which would accept the config
interface ToolConstructor {
options: ToolOptions | (config: ToolConfig) => ToolOptions
}
Facade would check if options is a method and pass the config if required. So external access via Facade is unaffected
It would simplify the flow and remove new preparedOptions entity
Contributor
Author
There was a problem hiding this comment.
Adopted – options is now ToolOptions | ((config: ToolConfig) => ToolOptions), resolved once in the facade. PreparedToolOptions and the ToolsManager plumbing are gone, prepare() is untouched.
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 <noreply@anthropic.com>
Reversean
changed the title
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 <noreply@anthropic.com>
gohabereg
reviewed
Aug 4, 2026
| * @param constructable - tool class being registered | ||
| * @param useToolOptions - second argument of `use(Tool, options)` | ||
| */ | ||
| function resolveStaticOptions(constructable: ToolConstructable, useToolOptions: ToolOptions): ToolOptions { |
Contributor
Author
There was a problem hiding this comment.
Moved to BlockToolFacade private methods.
gohabereg
approved these changes
Aug 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK tool contract so a tool’s static options can be derived synchronously from its per-registration config, and makes ToolConfig an SDK-owned type (instead of a legacy re-export that defaulted to any). Resolution/caching of factory-derived options is centralized in BaseToolFacade so multiple Core instances can register the same tool class with different configs without cross-contamination.
Changes:
- Make
ToolConfigan SDK-owned generic (object-defaulted) and propagate its import usage across SDK entities and in-repo tools. - Widen
BaseToolConstructor.optionstoOptions | ToolOptionsFactory(config), and cache the resolved static options per facade instance. - Add Jest coverage for factory resolution behavior and for
BlockToolFacade.toolbox/isReadOnlySupportedreading the resolved static options.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/tools/paragraph/src/index.ts | Switch ToolConfig import to @editorjs/sdk for the tool’s config typing. |
| packages/sdk/src/tools/facades/BlockToolFacade.ts | Read toolbox and isReadOnlySupported from per-facade resolved static options. |
| packages/sdk/src/tools/facades/BlockToolFacade.spec.ts | New characterization + factory-derived toolbox/readOnly tests. |
| packages/sdk/src/tools/facades/BaseToolFacade.ts | Resolve/caches static options (object or config-factory) per registration. |
| packages/sdk/src/tools/facades/BaseToolFacade.spec.ts | Add tests ensuring factory is called once, doesn’t mutate the class, and is per-facade. |
| packages/sdk/src/entities/InlineTool.ts | Move ToolConfig import to SDK-owned type location. |
| packages/sdk/src/entities/BlockTune.ts | Move ToolConfig import to SDK-owned type location. |
| packages/sdk/src/entities/BlockTool.ts | Move ToolConfig import to SDK-owned type location. |
| packages/sdk/src/entities/BaseTool.ts | Define SDK-owned ToolConfig + ToolOptionsFactory; widen options to union. |
| packages/sdk/src/entities/BaseTool.spec.ts | New type-level tests ensuring ToolConfig isn’t any and factories type-check. |
| openspec/changes/separate-tool-options-and-config/tasks.md | Spec-driven task checklist documenting intended implementation/verification. |
| openspec/changes/separate-tool-options-and-config/specs/sdk/spec.md | Delta spec describing new ToolConfig/options-factory requirements and scenarios. |
| openspec/changes/separate-tool-options-and-config/proposal.md | Proposal documenting motivation, breaking changes, and impact. |
| openspec/changes/separate-tool-options-and-config/design.md | Design decisions, constraints, risks, and migration plan for the contract change. |
| openspec/changes/separate-tool-options-and-config/.openspec.yaml | OpenSpec change metadata for the new change set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
static optionsis read off the tool class before any registration exists, whileToolConfigis resolved per registration. No option value can therefore depend onconfig — a field meant to drive toolbox entries or a shortcut stays inert, declared
in the tool's
Configtype and silently ignored.Two SDK changes:
static optionsmay now be a synchronous factory of the config:Options | ((config: ToolConfig) => Options). The facade resolves it once perregistration into a per-instance field, so nothing is written back onto the shared
class and two
Coreinstances can register one tool class with different configs.ToolConfigbecomes SDK-owned instead of anany-defaulted re-export from@editorjs/editorjs, which silently disabled checking for tools omitting the generic.prepare()is unchanged.packages/coreis untouched — resolution lives entirely inBaseToolFacade.BREAKING:
ToolConfigis imported from@editorjs/sdk;static optionswidens to a union.