From 67eab8c50913fadc1b51937f483aff47fedc6f57 Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Sun, 16 Aug 2026 10:22:13 +0000 Subject: [PATCH 1/2] fix(v4): register components under their merged config name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `registerComponent()` read the class's own static `config.name`, while the instance mounts under the merged one — the name `Base` publishes on `__base__`, and the name `resolveConfig()` already gives the family walk, the mount strategy and `@on(Class, type)`. A subclass extending a component with extra config and no rename therefore registered under `undefined`: no mount, no collision warning, and an `undefined` key left in the registry map. It now collides with the name it inherited, which is the loud first-wins path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9 --- packages/v4/src/registry.spec.ts | 31 +++++++++++++++++++++++++++++-- packages/v4/src/registry.ts | 16 ++++++++++++---- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/packages/v4/src/registry.spec.ts b/packages/v4/src/registry.spec.ts index ead4298ab..41c18ca12 100644 --- a/packages/v4/src/registry.spec.ts +++ b/packages/v4/src/registry.spec.ts @@ -1,5 +1,5 @@ -import { afterEach, describe, expect, it } from 'vitest'; -import { Base } from './Base.js'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { Base, type BaseConfig } from './Base.js'; import { DIAGNOSTICS, type ToolkitDiagnosticDetail } from './diagnostic-contract.js'; import { EVENTS } from './events.js'; import { registerComponent } from './registry.js'; @@ -226,4 +226,31 @@ describe('registry', () => { expect(calls).toEqual(['before:terminated', 'after:mounted:before=false']); }); + + it('registers a subclass under its merged name, not its own static config', async () => { + class Named extends Base { + static config = { name: 'MergedName' }; + } + + class Extended extends Named { + // No `name`: the merged config inherits `MergedName`, which is what the + // instance mounts under, so registration has to see the collision. + // @ts-expect-error `name` is missing, as it is in untyped sources. + static config: BaseConfig = { options: { extra: Boolean } }; + } + + registerComponent(Named); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + registerComponent(Extended); + const el = document.createElement('div'); + el.setAttribute('data-component', 'MergedName'); + document.body.append(el); + await settle(); + + expect(warn).toHaveBeenCalledWith( + `[js-toolkit:${DIAGNOSTICS.registry.conflict}] "MergedName" is already registered; the incoming declaration was ignored.`, + ); + expect(getInstance(el, 'MergedName')).toBeInstanceOf(Named); + warn.mockRestore(); + }); }); diff --git a/packages/v4/src/registry.ts b/packages/v4/src/registry.ts index 49b129546..f0b0557aa 100644 --- a/packages/v4/src/registry.ts +++ b/packages/v4/src/registry.ts @@ -188,9 +188,16 @@ if (!registryState.isReplacementListenerAttached) { }); } -/** Register a component and its merged family, then scan matching elements. */ +/** + * Register a component and its merged family, then scan matching elements. + * + * The name comes from the merged config, like the instance's `$id` and the + * `__base__` key it publishes itself under: a subclass which extends a + * component with extra config and forgets to rename would otherwise register + * under `undefined` instead of colliding with the name it inherited. + */ export function registerComponent(ComponentClass: BaseConstructor): void { - const { name } = ComponentClass.config; + const { name } = resolveConfig(ComponentClass); if (registry.has(name)) { if (registry.get(name) !== ComponentClass) { warnOnce( @@ -268,12 +275,13 @@ function importComponent(name: string, target?: Element): Promise { if (!ComponentClass) { throw new TypeError(`"${name}" did not resolve to a component class.`); } - if (ComponentClass.config.name !== name) { + const resolvedName = resolveConfig(ComponentClass).name; + if (resolvedName !== name) { warnOnce( entry.load, name, 'registry.lazy-name-mismatch', - `"${name}" resolved to a component named "${ComponentClass.config.name}".`, + `"${name}" resolved to a component named "${resolvedName}".`, { component: name, target }, ); } From 8984b60705386cf77f939637ded69b4010650d3f Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Sun, 16 Aug 2026 10:22:56 +0000 Subject: [PATCH 2/2] docs(v4): answer withExtraConfig with plain subclassing v3's `withExtraConfig()` existed because v3 read a class's own static `config`. v4 merges config along the prototype chain, so extending a component with different config is `extends` plus a `static config`, and `@component()` already takes that config object. Adds the spec proving it against the three `@studiometa/ui` call sites, and records the two v3 behaviours v4 refuses: the auto-rename on collision, and the deep merge of the config. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9 --- packages/v4/DESIGN.md | 23 ++- packages/v4/src/config-extension.spec.ts | 211 +++++++++++++++++++++++ 2 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 packages/v4/src/config-extension.spec.ts diff --git a/packages/v4/DESIGN.md b/packages/v4/DESIGN.md index 3cb7b983f..eb6050e5d 100644 --- a/packages/v4/DESIGN.md +++ b/packages/v4/DESIGN.md @@ -97,7 +97,28 @@ The two remaining regressions are understood rather than outstanding. `$emit` pa `$config` walks the prototype chain and merges every config it finds, so extending a component keeps what its parents declared — the crash reported in #627. `refs`, `options` and `components` all merge (v3 merged only `options` and `emits`); scalar keys stay overridable by the most derived class, and a subclass restating a `components` key wins for that key alone. An intermediate class should annotate `static config: BaseConfig`, otherwise TypeScript infers a literal type its subclasses must match. -**The registry reads the merged config too**, and reads it before any instance exists, which is why `resolveConfig()` is exported from `Base.ts`. It resolves the mount strategy of a pair (§11b) and registers the family of `config.components` (§11d) from the merged set, not the class's own static. Every subclass declares a `static config` if only for its `name`, so reading the own static made a subclass fall back to `eager` and register nothing its base declared — while its instances still announce and query those children through `$config`. A `() => import(…)` child has no registration path besides this one, so it went missing outright. +**The registry reads the merged config too**, and reads it before any instance exists, which is why `resolveConfig()` is exported from `Base.ts`. It resolves the mount strategy of a pair (§11b), registers the family of `config.components` (§11d) and takes the name a class registers under, all from the merged set rather than the class's own static. Every subclass declares a `static config` if only for its `name`, so reading the own static made a subclass fall back to `eager` and register nothing its base declared — while its instances still announce and query those children through `$config`. A `() => import(…)` child has no registration path besides this one, so it went missing outright. The name had the same shape of bug: a subclass that declared options and forgot to rename registered under `undefined` instead of colliding with the name it inherited. + +### Extending a component with different config — `withExtraConfig` is `extends` + +v3's `withExtraConfig(Class, config, deepmergeOptions)` returned a subclass whose `config` was the original deep-merged with an override, renamed when the name collided. It existed because v3 read a class's own static `config`: a subclass could not add one option without restating everything its parent declared. Merging along the prototype chain removes the reason, so **v4 ships no equivalent and does not need one** — the operation is a class declaration: + +```js +class MapboxNavigationControl extends AbstractMapboxControl { + static config = { + name: 'MapboxNavigationControl', + options: { showCompass: Boolean, showZoom: Boolean }, + }; +} +registerComponent(MapboxNavigationControl); +``` + +That is the whole translation of all three `@studiometa/ui` call sites (`MapboxNavigationControl`, `MapboxGeolocateControl`, `MapboxFullscreenControl`), each of which overrides `createControl()` and so needs a class body regardless. `@component({ name, options })` is the same thing with the registration folded in, and it takes a config object already — there is nothing to extend. The base's `position` option keeps its default, its refs and its `components` come along, and the base itself is left untouched. A class you cannot edit is extended in expression position: `registerComponent(class extends Vendor { static config = { name: 'CompactVendor', … } })`. `src/config-extension.spec.ts` holds the proof. + +Two v3 behaviours are deliberately not reproduced: + +- **The auto-rename.** v3 renamed a colliding result to `WithExtraConfig`, a name nobody writes in HTML. `name` is required by `BaseConfig`, and the registry is first-wins-and-warn (§11f item 3), so a forgotten rename is a diagnostic rather than a machine-invented token. All three ui call sites already name their extension, so the branch was dead there. +- **The deep merge.** v3 took npm `deepmerge` plus a caller-supplied options object. Core ships its own `deepmerge` now (`utils/deepmerge.ts`), but config is **not** where it belongs: `Base` merges config one level on purpose, and an option definition is a unit — a derived class restating `theme` restates its type _and_ its default, which is what "this option is different here" means. Deep-merging would also have to reach into `default` factory functions, which it treats as opaque values. The knob v3 exposed for tuning that merge has no v4 equivalent because the merge it tuned is gone. ### The public surface is typed, and free diff --git a/packages/v4/src/config-extension.spec.ts b/packages/v4/src/config-extension.spec.ts new file mode 100644 index 000000000..e449f355d --- /dev/null +++ b/packages/v4/src/config-extension.spec.ts @@ -0,0 +1,211 @@ +/** + * v3 shipped `withExtraConfig(Class, config, deepmergeOptions)` because v3 read + * a class's **own** static `config`: a subclass had to restate what its parent + * declared, so a decorator did the merge and renamed the result. v4 resolves + * `config` along the prototype chain (`resolveConfig()`), which makes the + * decorator a plain `extends` plus a `static config`. + * + * These specs pin that equivalence against the three `@studiometa/ui` + * `withExtraConfig` call sites — the mapbox controls — and against the two + * v3 behaviours v4 deliberately dropped: the auto-rename on collision and the + * deep merge of the config. + */ +import { afterEach, describe, expect, expectTypeOf, it, vi } from 'vitest'; +import { Base, type BaseConfig, type BaseProps } from './Base.js'; +import { component } from './decorators.js'; +import { DIAGNOSTICS } from './diagnostic-contract.js'; +import { registerComponent } from './registry.js'; +import { getInstance, resetDom, settle, TodoItem } from './test-utils.js'; + +afterEach(resetDom); + +interface ControlProps extends BaseProps { + $options: { position: string }; +} + +/** + * `AbstractMapboxControl`: the shared option, one ref and one family entry. + * It is never registered, which is all "abstract" means to the registry. + */ +class AbstractControl extends Base { + static config: BaseConfig = { + name: 'AbstractControl', + refs: ['handle'], + options: { position: { type: String, default: 'top-right' } }, + components: { TodoItem }, + }; + + /** Mirrors the ui getter which spreads `$options` into the mapbox control. */ + controlOptions(): Record { + const { position: _position, ...options } = this.$options; + return options; + } +} + +function render(name: string, attributes: Record = {}): HTMLElement { + const el = document.createElement('div'); + el.setAttribute('data-component', name); + for (const [attribute, value] of Object.entries(attributes)) { + el.setAttribute(attribute, value); + } + document.body.append(el); + return el; +} + +describe('extending a component with extra config', () => { + it('adds options to the base and keeps everything the base declared', async () => { + interface NavProps extends ControlProps { + $options: ControlProps['$options'] & { showCompass: boolean; showZoom: boolean }; + } + + class NavControl extends AbstractControl { + static config: BaseConfig = { + name: 'NavControl', + options: { showCompass: Boolean, showZoom: Boolean }, + }; + } + + registerComponent(NavControl); + const el = render('NavControl', { 'data-option-show-compass': '' }); + await settle(); + + const instance = getInstance(el, 'NavControl'); + expect(instance.$config.name).toBe('NavControl'); + expect(Object.keys(instance.$config.options ?? {})).toEqual([ + 'position', + 'showCompass', + 'showZoom', + ]); + // The inherited option keeps its default, the added ones read the DOM. + expect(instance.$options.position).toBe('top-right'); + expect(instance.controlOptions()).toEqual({ showCompass: true, showZoom: false }); + expectTypeOf(instance.$options.showCompass).toEqualTypeOf(); + expectTypeOf(instance.$options.position).toEqualTypeOf(); + + // Refs and the child family come along; the base is left untouched. + expect(instance.$config.refs).toEqual(['handle']); + expect(Object.keys(instance.$config.components ?? {})).toEqual(['TodoItem']); + expect(Object.keys(AbstractControl.config.options ?? {})).toEqual(['position']); + }); + + it('renames the base without declaring anything else', async () => { + class FullscreenControl extends AbstractControl { + static config: BaseConfig = { name: 'FullscreenControl' }; + } + + registerComponent(FullscreenControl); + const el = render('FullscreenControl', { 'data-option-position': 'bottom-left' }); + el.innerHTML = ''; + await settle(); + + const instance = getInstance(el, 'FullscreenControl'); + expect(instance.$config.name).toBe('FullscreenControl'); + expect(instance.$options.position).toBe('bottom-left'); + expect(instance.$refs.handle).toBe(el.firstElementChild); + }); + + it('mounts two extensions of one base side by side, each under its own name', async () => { + class LeftControl extends AbstractControl { + static config: BaseConfig = { + name: 'LeftControl', + options: { position: { type: String, default: 'top-left' } }, + }; + } + + class RightControl extends AbstractControl { + static config: BaseConfig = { name: 'RightControl' }; + } + + registerComponent(LeftControl); + registerComponent(RightControl); + const left = render('LeftControl'); + const right = render('RightControl'); + await settle(); + + // A restated option replaces the parent definition whole; it does not + // merge into it, so the derived default wins with nothing left behind. + expect(getInstance(left, 'LeftControl').$options.position).toBe('top-left'); + expect(getInstance(right, 'RightControl').$options.position).toBe('top-right'); + }); + + it('extends a class it cannot edit, in expression position', async () => { + class Vendor extends Base { + static config: BaseConfig = { name: 'Vendor', options: { size: { type: String } } }; + } + + registerComponent(Vendor); + registerComponent( + class extends Vendor { + static config: BaseConfig = { name: 'CompactVendor', options: { compact: Boolean } }; + }, + ); + const el = render('CompactVendor', { 'data-option-compact': '' }); + await settle(); + + const instance = getInstance(el, 'CompactVendor'); + expect(instance).toBeInstanceOf(Vendor); + expect(instance.$options.compact).toBe(true); + expect(Object.keys(instance.$config.options ?? {})).toEqual(['size', 'compact']); + }); + + it('declares and registers the extension in one step with `@component`', async () => { + @component({ name: 'DecoratedControl', options: { showZoom: Boolean } }) + class DecoratedControl extends AbstractControl {} + + const el = render('DecoratedControl'); + await settle(); + + const instance = getInstance(el, 'DecoratedControl'); + expect(instance).toBeInstanceOf(AbstractControl); + expect(instance.$config.name).toBe('DecoratedControl'); + expect(Object.keys(instance.$config.options ?? {})).toEqual(['position', 'showZoom']); + expect(instance.$options.position).toBe('top-right'); + }); +}); + +describe('what v3 did and v4 does not', () => { + it('requires the rename instead of inventing one, and warns on the collision', async () => { + class Widget extends Base { + static config: BaseConfig = { name: 'Widget' }; + } + + class UnnamedWidget extends Widget { + // v3 renamed the collision to `WidgetWithExtraConfig`. v4 makes `name` + // required and the registry first-wins, so the rename is the author's. + // @ts-expect-error `name` is missing. + static config: BaseConfig = { options: { loud: Boolean } }; + } + + registerComponent(Widget); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + registerComponent(UnnamedWidget); + const el = render('Widget'); + await settle(); + + expect(warn).toHaveBeenCalledWith( + `[js-toolkit:${DIAGNOSTICS.registry.conflict}] "Widget" is already registered; the incoming declaration was ignored.`, + ); + expect(getInstance(el, 'Widget')).not.toBeInstanceOf(UnnamedWidget); + warn.mockRestore(); + }); + + it('does not deep merge an option definition it restates', () => { + class Themed extends Base { + static config: BaseConfig = { + name: 'Themed', + options: { theme: { type: String, default: 'light' } }, + }; + } + + class Retyped extends Themed { + // Only the type is restated: v3's deep merge kept the parent default, + // v4 replaces the definition, so the option falls back to the empty + // string. Restate the default when the derived class wants one. + static config: BaseConfig = { name: 'Retyped', options: { theme: String } }; + } + + const instance = new Retyped(document.createElement('div')); + expect(instance.$config.options?.theme).toBe(String); + expect(instance.$options.theme).toBe(''); + }); +});