feat(core): report missing modules when an option or API needs one#1491
Merged
Conversation
Setting an enterprise option on the free package did nothing and said
nothing: the service slot is empty, every call site is `?.`, so it
short-circuits silently. A user could not tell a broken feature from a
missing package.
Report when intent is declared — an option set, or an api command called:
dockview: `overflow.mode: 'wrap'` requires the "MultiRowTabs" module,
which ships in dockview-enterprise.
npm install dockview-enterprise
import 'dockview-enterprise';
Interaction stays silent. A right-click reaching an absent ContextMenu
module doesn't mean the app wanted one, and the `?.` call site can't tell
a missing module from a feature nobody asked for.
- optionsModules.ts: rules table mapping options to the modules that
implement them, validated at construction and on updateOptions. A flat
list rather than a keyed record because options nest and gate on values
— `overflow.mode: 'wrap'` needs MultiRowTabs while `overflow.search`
needs AdvancedOverflow, and the message names the exact path. Reasons
are grouped so each missing module reports once however many of its
options are set.
- modules.ts: ENTERPRISE_MODULE_NAMES + one message builder, so core
modules never mention the paid package and enterprise ones always carry
the fix. assertModule delegates to it.
- Guard commands, not queries: api.undo() asked for something to happen;
canUndo asked a question and `false` is a truthful answer.
- addEdgeGroup keeps throwing (its return type is non-optional, so there
is no no-op to degrade to) but now raises the same message instead of
logging one and throwing a thinner second one.
- enterpriseModuleNames.spec: fails if ENTERPRISE_MODULE_NAMES drifts from
dockview-enterprise's Modules in either direction. Core can't import
enterprise to derive the list, so this test is the only thing holding
the mirror in sync.
Validation reads the options the caller passed, never the merged set:
every key of DockviewOptions is present as `undefined` on `this.options`,
so a presence test there would fire for every rule.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`dockToEdgeGroups` is an enterprise feature — features.mdx ticks only the Enterprise column and autoEdgeGroups.mdx is `enterprise: true` — but core carried a single-band fallback in `dockToLayoutEdge` that performed the dock whenever AutoEdgeGroup was absent. A free user setting the option dropped a panel on a root edge and got an edge group: the paid feature, minus only the two-band drag-reveal affordance. Remove the fallback. The option is now inert without the module and a root-edge drop splits the grid, as it does when the option is unset. Enterprise behaviour cannot change: the removed block's own condition included `!autoEdgeGroupService`, making it dead code whenever the module is registered — those drops already returned earlier via the module's `onWillDrop.preventDefault`, or fell through to the same move below. The enterprise suite passes untouched. Add the core tests that never existed for this path: they fail against the old fallback and pass now. Not addressed here: `resolveRootOverlayModel` still widens the root drop band 10px -> 32px from the option alone, so a free build gets a wider activation band that plainly splits the grid. Gating that on the service is not safe today — RootDropTargetService resolves the band in its own constructor during `_moduleRegistry.initialize()`, and core modules initialise before the enterprise ones, so the service would read `autoEdgeGroupService` as absent and collapse the band to 10px for paying users too. It needs the check deferred to postConstruct; tracked separately. The band is cosmetic — the feature itself no longer leaks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`keyboardNavigation` is read by nothing in core and only by the enterprise keyboard modules, but had no rule — so setting it on the free package did nothing and said nothing. Exactly the failure the rules table exists to prevent, shipped in the table itself. Add the rule, and stop it recurring. The table is hand-written and core cannot derive it (it can't import the modules it might be missing), so each module now declares the options that must name it (`DockviewModule.options`), and a test in dockview-enterprise pins the two together in both directions: - an option a module claims with no rule -> the user gets silence - a rule naming a module for an option it doesn't own -> the wrong module gets named, or a free feature gets billed as paid The test reproduces this bug when the new rule is removed. Rules gain `optionKey` so the test can match them without parsing prose; `reason` still carries the precise path for the message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes from a cold review of the branch.
`dndGuide: false` demanded the paid package. The rule tested presence
(`!= null`) on a `boolean | {...}` option, so explicitly turning the
compass *off* billed the user for it — the exact bug this branch exists to
prevent, introduced by it. `smartGuides: false` did the same: `false`
isn't in its type, but JS callers and prop-forwarding bindings reach it,
and `false != null` fell through to `false.enabled`. Both now test
truthiness, matching every sibling rule and the contract the file states.
The disabled-is-silent test hand-listed five rules and missed the broken
one, so it is now enumerated per rule. It found the `smartGuides` case
immediately.
Finish the dock-to-edge gate. Removing the fallback left the option
widening the root activation band 10px -> 32px with no module check, so a
free build got a 3.2x larger *grid-split* trigger and nothing else — a
regression, and the comment claiming the option was "inert" was false.
The band is now gated on the affordance being registered.
That gate has a trap worth knowing: RootDropTargetService resolves the
band in its own constructor, during module initialisation, and core's
modules initialise before the enterprise ones — so `hasEdgeDragReveal` is
always false at that point, however the component is configured. Gating
naively would collapse the band to 10px for paying users too. The
re-resolve is deferred to the module's `init` (postConstruct); a test in
dockview-enterprise pins it, and fails with 10 != 32 if the deferral goes.
Also:
- `options.ts` still advertised dockToEdgeGroups as needing only the free
RootDropTarget + EdgeGroup modules — the IDE hover contradicted the
diagnostic, so a user would read the error as a false positive.
- Comments in moduleContracts and revealEdgeGroupWithData still described
the deleted fallback; setPanelPinned still promised a warn that's gone.
- assertModule now states both rules for guarding an entry point, since
the branch applied opposite reasoning to setPanelPinned and undo/redo.
They differ by reachability: undo() works with no option set, so no rule
can have fired; setPanelPinned can't be reached until the option is set.
- The completeness spec claimed to prevent silent gaps. It can't: coverage
derives from a hand-written declaration, so an option with neither a
declaration nor a rule still passes. Docstring now says so.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With core's single-band fallback removed, resolve the root edge overlay band honestly at construction: pass `false` rather than `host.hasEdgeDragReveal`, whose service may not exist yet during module init. The `init` hook re-resolves the real band afterwards. Update the OPTION_MODULE_RULES rationale to say core implements no part of dock-to-edge, and pin the tier test to the concrete default activation size instead of comparing two free builds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Join pinned panel ids on the ASCII unit separator (0x1f) instead of a space when building the change-detection key, so ids containing the delimiter can't collide and silently skip a DOM rebuild. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove em dashes from the comments added with the missing-module diagnostics work and rephrase a few passages to describe the code as it stands rather than what changed. Comment and test-description text only; no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pinEdgeGroup, autoHideEdgeGroup and peekEdgeGroup reached the enterprise AutoHideEdgeGroup service through a silent optional call. Edge groups are a free feature, so a build can hold one and call these commands without setting the autoHideEdgeGroups option that would otherwise name the module, leaving the command a silent no-op. Route them through assertModule so they report the missing module, matching undo/redo. Add apiCommandModules.spec covering the guarded commands, that queries (canUndo/canRedo) stay silent, and that dedup holds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pull the plural/singular verb choice out of the message template so the outer ternary no longer nests one. Output is unchanged. Addresses SonarQube typescript:S3358. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdbUtXC52FHGhPf6WxsHvM
|
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
1 participant
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.
Description
Adds diagnostics so that when an option or API command requires a module that isn't registered, dockview reports which module (and package) provides the fix instead of failing silently. Previously a feature gated behind an enterprise module could be requested with no feedback when that module was absent.
Key changes:
optionskeys that must report the module when it isn't registered — the module's half of the contract with core'sOPTION_MODULE_RULES. Declared explicitly (rather than inferred) so a test keeps the two in sync.ENTERPRISE_MODULE_NAMESset of the module names shipped bydockview-enterpriseso a missing-module message can name the package that provides the fix, without core importing those modules.dockview-enterprise: services annotate their module name, and the pinned-tab rebuild key uses a unit separator to avoid key collisions.Type of change
Affected packages
dockview-coredockview(vanilla JS)dockview-reactdockview-vuedockview-angulardocsAlso affects
dockview-enterprise(not listed in the template).How to test
yarn testinpackages/dockview-core. New specs cover the behaviour:optionsModules.spec.ts— options that require a module produce a diagnostic naming it.apiCommandModules.spec.ts— API commands report the missing module.dockToEdgeGroupsTier.spec.ts— dock-to-edge gating for free vs enterprise builds.enterpriseModuleNames.spec.ts(enterprise) — keeps the option/rule/module lists in sync.edgeDragRevealBand.spec.ts(enterprise) — edge-drag reveal band behaviour.Checklist
yarn lint:fixpassesyarn formatpassesnpm run genhas been run and generated files are up to dateyarn testpasses