Settle whether @component() needs to copy the inherited config - #832
Merged
Merged
Conversation
`@component()` writes `{ ...value.config, ...config }`, so a class with no own
static `config` copies its parent's keys down. #823 left it alone as probably
redundant, because `resolveConfig()` already merges the prototype chain.
Three differential specs settle it. The merged config is identical whether the
own config carries the inherited keys or not, so the copy is redundant for
every value. It is not redundant for `isBaseConstructor()`, which reads
`config.name` straight off the class: a config written without a name — what an
untyped subclass adding config to its parent's produces — leaves the class
registered under the name it inherited yet rejected by every brand check.
A fourth pins which config wins when a class declares both the decorator and a
static field: the field, because its initializer runs after class decorators.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9
The copy adds nothing to the merged config, so the next reader will reach for it again. Name the one thing it carries — the inherited `config.name` that `isBaseConstructor()` reads off the class — and the three call sites that refuse a class without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. ✅ No export size changes. Unchanged (388)@studiometa/js-toolkit
@studiometa/js-toolkit-v4
|
Code ReviewRisk: Low — The change is safe to merge and adds coverage and documentation explaining why The new specs verify that the copy does not alter the resolved configuration, preserves brand recognition for configs without a name, avoids mutating parent config objects, and documents static-field overwrite behavior. The decorator implementation remains functionally unchanged apart from the explanatory comment. No issues found. Review usage: 53,850 in (43,599 cached) / 1,403 out tokens — $0.0128 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 4adae7a. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #832 +/- ##
=======================================
Coverage 97.16% 97.16%
=======================================
Files 170 170
Lines 4133 4133
Branches 1152 1151 -1
=======================================
Hits 4016 4016
Misses 106 106
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
v3 mount benchmarksBase and head measured on this runner, alternating over 3 rounds each; every value is the median of the round medians. Running both sides on one machine is what removes cross-machine noise — a cached baseline from another runner would put it back. A move under 25%, or on a benchmark under 5 ms, is not reported as a change: it is inside the measured noise of a shared runner. No benchmark moved beyond the noise floor. Within noise (10)
|
v4 mount benchmarksBase and head measured on this runner, alternating over 3 rounds each; every value is the median of the round medians. Running both sides on one machine is what removes cross-machine noise — a cached baseline from another runner would put it back. A move under 25%, or on a benchmark under 5 ms, is not reported as a change: it is inside the measured noise of a shared runner.
Within noise (15)
|
This was referenced Aug 16, 2026
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.
#823 left a note on
@component():value.config = { ...value.config, ...config }copies a parent's config onto the child as its own, andresolveConfig()already mergesconfigalong the prototype chain, so the copy looked redundant. Three other PRs were touchingdecorators.tsat the time. They have merged, so here is the answer.It is not redundant. The code stays, with a comment saying why.
The evidence
Four specs in
decorators.spec.ts, written against the current code, then re-run againstvalue.config = config(the copy removed). Only one of them changes.The copy adds no value to the merged config. A subclass whose own config carries the inherited keys and one whose own config carries only the declared keys resolve to the same
refsunion, the sameoptions, the samecomponents, and the same inheritedmountStrategy.resolveConfig()collects onlyObject.hasOwn(current, 'config')levels and merges them, which is exactly the work the copy duplicates. This spec passes either way.The copy is what keeps the class recognisable.
isBaseConstructor()readsconfig.namestraight off the class. It cannot callresolveConfig()— that lives inBase, which imports the brand. A class with no ownconfigpasses the check because it reads its parent's through the prototype chain. A decorated one only passes because the decorator carried the inherited name into the config it wrote.What would have broken
The case is a config written without a
name.BaseConfigrequires one, so TypeScript catches it; untyped sources reach it by extending a component and adding config without renaming — the exact case #823 made register under the inherited name:Without the copy,
Child.configis{ options: { two: String } }.resolveConfig()still resolves the name toParent, so the class registers and mounts and collides loudly — butisBaseConstructor(Child)is nowfalse, and the three places that ask reject it:registerFamily()— aconfig.componentsentry warnscomponent.invalid-family-declarationand is dropped@on(Child, 'event')— throwsTypeErrorresolveComponentClass()— a lazily imported module "did not resolve to a component class"A class that registers and mounts, but that no parent can declare and no handler can target. The copy is one shallow spread standing between the decorator and that split.
One defect found, not fixed here
Declaring both
@component({...})andstatic config = {...}on the same class silently drops everything the decorator declared: static field initializers run after class decorators, so the field replaces the decorator's write wholesale. The last spec pins which one wins so the loss is at least known. Fixing it means moving the write intocontext.addInitializer(), which runs after the static fields — a change to when a class's config exists, not a cleanup, so it is not folded in here.npm run lint,npm run lint:types,npm run test:v4(1078 passing) andnpm run check:packageare green.🤖 Generated with Claude Code
https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9