fix(v4): merge @component with a static config instead of dropping one - #835
Merged
Merged
Conversation
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. @studiometa/js-toolkit-v4
Unchanged (379)@studiometa/js-toolkit
@studiometa/js-toolkit-v4
|
Code ReviewRisk: Low — No blocking issues found; the change is safe to merge. Updates Review usage: 55,115 in (42,476 cached) / 1,278 out tokens — $0.0139 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit b0608b4. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #835 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 170 170
Lines 4143 4143
Branches 1152 1152
=======================================
Hits 4026 4026
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. No benchmark moved beyond the noise floor. Within noise (18)
|
A class decorator runs before static field initializers, so a class declaring
both `@component({ … })` and a `static config` lost the decorator's half of the
declaration the moment the field initialised. Nothing was reported: `refs`,
`options` and `components` written in the decorator simply were not there.
`context.addInitializer()` runs after those fields — the first moment both
declarations exist — and still inside the class definition, so the config is
final before the statement after the class, `registerComponent()` at module
scope included. Merge them there, by the rules `resolveConfig()` already
applies along a prototype chain: refs union, `options` and `components` merge
entry by entry, a declared value overrides. No second set of rules to learn.
The decorator is applied last, the direction it already has over the inherited
config, so one rule covers both. That precedence is only observable when the
two declare one key differently, which is an authoring mistake and not an
intent, so it is reported as the new `component.config-conflict` warning
naming each key. Declaring the same value twice, or disjoint keys, merges
silently — there is nothing for the author to resolve.
The merged config still carries the inherited `config.name` when neither side
declares one, which is what keeps `isBaseConstructor()` passing on a class an
untyped source extends without renaming.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9
The decorator section listed `@component` as sugar over `static config` and said nothing about a class carrying both. State the merge, its precedence, and why the conflict warning exists rather than a silent rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9
titouanmathis
force-pushed
the
fix/v4-component-decorator-static-config
branch
from
August 16, 2026 19:41
b0608b4 to
d79c28f
Compare
66 tasks
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.
Stacks on #832 — branched off
refactor/v4-component-decorator-config, and it should land after it. #832 added the spec pinning today's behaviour; this PR changes that behaviour and rewrites the spec to describe the new one rather than delete it. The diff againstmainwill read cleanly only once #832 is in.The defect
A class decorator runs before static field initializers, so the field's object replaced the one the decorator wrote. Everything the field did not repeat was lost, silently.
What it does now: merge, decorator last, warn on a real conflict
context.addInitializer()runs after the static fields — the first moment both declarations exist — and still inside the class definition. The two are merged there.resolveConfig()already applies along a prototype chain:refsunion,optionsandcomponentsmerge entry by entry, a declared value overrides. Two declarations on one class behave exactly like two declarations on a chain, so there is no second set of rules to learn — which matters for an audience that reads the docs and not the implementation.@componentalready spread its argument over the inherited config ({ ...value.config, ...config }). Keeping the same direction over the own static field means one rule — the decorator argument is the last writer — instead of two opposite ones.component.config-conflictwarning naming each key (name, mountStrategy, options.open). That is the only case where the precedence rule is observable, and two different values for one key is an authoring mistake rather than an intent. Declaring the same value twice, or disjoint keys, merges silently: there is nothing for the author to resolve, and warning there would punish the legitimate pattern of a decorator that names and registers beside a static config carrying a long options table.refsnever conflict — a union loses nothing.What was rejected
Diagnose only, keep one side. A warning still has to pick a winner, so it buys the same precedence rule plus a discarded declaration. It is the strictly worse half of what is shipped here.
Warn on any overlap, conflicting or not. Both sides almost always repeat
namewith the identical value; warning on that is noise on the very pattern the fix is meant to support.Field wins. It would need its own rule, opposite to the one
@componentalready applies to the inherited config, and a component mounting under a name other than the one written in@component({ name })reads as a bug.Constraints held
registerComponent()on the very next line, and asserts the merged config, the mount, the option value and the ref. Class initializers run while the class definition evaluates, so nothing moves later than it was.isBaseConstructor()keeps passing. The eager write is unchanged, and the merged object carries the inheritedconfig.nameback when neither side declares one. Settle whether @component() needs to copy the inherited config #832's brand spec is untouched, and a second spec covers the same case reached through a nameless static field.Verification
npm run lint,npm run lint:types,npm run test:v4(1083 passed) andnpm run check:packageall pass frompackages/v4.🤖 Generated with Claude Code
https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9