Convergent restore-default for reflected props#107
Convergent restore-default for reflected props#107DmitrySharabin wants to merge 14 commits intoprops-batched-drainfrom
Conversation
3b7db06 to
7c714d9
Compare
| }, options); | ||
| signal.subscribe((newValue, oldValue) => { | ||
| this.#onComputedChange(element, source, newValue, oldValue); | ||
| let fromDefault = rawSignal.value === undefined; |
There was a problem hiding this comment.
This reminds me, defaults are one case where updated() consumers may want to read either the internal value or the getter value. They can always get the current getter value via the actual getter, but not the old value.
8f336dd to
38245b9
Compare
ec16bcb to
12ce09f
Compare
38245b9 to
d85d3c9
Compare
6229f8d to
5c74a8e
Compare
5c74a8e to
87bff0b
Compare
d85d3c9 to
b87044b
Compare
|
Before I review the code, one thing I want to make sure (and we should have tests for) is that the behavior is as expected. Suppose you have element
|
87bff0b to
039910e
Compare
b87044b to
aeba6f9
Compare
039910e to
418f61e
Compare
aeba6f9 to
35fdbfc
Compare
418f61e to
ec7c875
Compare
35fdbfc to
ed9e86d
Compare
ec7c875 to
00bcd97
Compare
ed9e86d to
5c2e375
Compare
00bcd97 to
e6da2b6
Compare
5c2e375 to
fb9c3ec
Compare
In Prop.set, collapse null → undefined when source is "attribute". Attribute removal now reverts the prop to its natural empty state — the default if there is one, otherwise undefined — matching native HTML attribute behavior. ## Why #105 made defaults stop reflecting on mount, but the inverse path was still broken: removeAttribute() arrives via attributeChangedCallback as JS null, parse() returns it unchanged, and the Computed wrapper only falls through to the default on undefined (not null). The prop landed at null instead of restoring the default. ## Scope Only the attribute path collapses null → undefined. Property writes of null remain a legitimate user value. A second open question from PR #107 review — "explicit write equal to default should still reflect" — ships as a skipped test in the same group as a placeholder for follow-up.
Decouple "user intent commit" from "Computed value change". Reflection fired only from the Computed subscriber; the subscriber doesn't fire when an explicit write happens to match the prior default-resolved value (Computed equality dedupe), so reflection silently dropped. Extract #reflect from #onComputedChange. Call it inline in Prop#set after the rawSignal write, computing the settled (post-convert) value manually — without forcing a synchronous read of signal.value, which would notify the Computed's subscribers in mid-write and break the batched-drain coalescing that PR #102 established. The subscriber path still calls #reflect for reactive cases (default re-eval, convert dep change, get re-eval). For value-changing user writes, both paths run; the second is a no-op (oldAttributeValue matches the just-written attributeValue). ## Tests - Un-skip "Explicit write equal to default still reflects" — was the open question from PR #107 review (Lea's scenario #2). - Rename "Assigning current default-resolved value is a no-op" to "...is a no-op for events" — the test only asserts event semantics, and reflection is no longer a no-op for this case. ## Why not signal.value Reading signal.value forces synchronous compute. The compute notifies the Computed's subscribers synchronously, which queues the prop's propchange event before the dirty-flush microtask runs. If a drain microtask is already pending (e.g., from a prior sync drain that left #drainScheduled set), that drain dispatches the early-fired prop alone, splitting a multi-prop cascade across two updated() calls. Computing settledValue inline preserves the dirty-flush semantics.
|
I added the corresponding tests. I just would like to discuss one case. Let's consider the following definition of import NudeElement from "nude-element";
class XFoo extends NudeElement {
static props = {
bar: {
type: Number,
default: 5,
reflect: true, // spelled out for clarity
},
};
}
customElements.define("x-foo", XFoo);
This is how it works now. No changes required.
Because With the recent changes, it also works as you described. The reflection is decoupled from the computed dedupe via an inline path in
Writing Returning
It also works with the recent changes. |
|
Yeah, the important bit is to distinguish "explicitly set to default value" from "set to default value because no value has been set". E.g. a button component may default to
|
Pre-signals, storage started `undefined`, so the equals check at the top of `Prop#set` was always false on the first user write — regardless of whether the value matched the default. Reflection always reached the DOM. The signals Computed dedupes against its cached default-resolved value, so an explicit write equal to the prior default never fires the subscriber, and reflection silently drops. Add a `forceNotify` option to `Signal`: subscribers fire on every write, even when `equals` reports no change. The cached value still respects `equals` (no-op writes don't update it), so the existing spec.equals contract — Computed cache reflects only meaningful changes — is preserved. Drop the redundant equals guard in `Computed#compute` since the setter now decides. Use `forceNotify: true` on the prop Computeds. `#onComputedChange` gates the propchange event on actual value change to keep main's value-change semantics. Reflection runs whenever the subscriber fires (driven by user intent); the event still tracks the value. Tests for this scenario land in #107. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-signals, storage started `undefined`, so the equals check at the top of `Prop#set` was always false on the first user write — regardless of whether the value matched the default. Reflection always reached the DOM. The signals Computed dedupes against its cached default-resolved value, so an explicit write equal to the prior default never fires the subscriber, and reflection silently drops. Add a `forceNotify` option to `Signal`: subscribers fire on every write, even when `equals` reports no change. The cached value still respects `equals` (no-op writes don't update it), so the existing spec.equals contract — Computed cache reflects only meaningful changes — is preserved. Drop the redundant equals guard in `Computed#compute` since the setter now decides. Use `forceNotify: true` on the prop Computeds. `#onComputedChange` gates the propchange event on actual value change to keep main's value-change semantics. Reflection runs whenever the subscriber fires (driven by user intent); the event still tracks the value. Tests for this scenario land in #107. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three new tests cover the fixes in this PR (T0 already covered by the "Default does NOT reflect on mount" tests): - "removeAttribute restores default" (Final value group) — verifies the null → undefined collapse in Prop#set. - "removeAttribute clears the reflected attribute" (Attribute reflection group) — verifies the gate at #onComputedChange suppresses default re-reflection. - "Explicit write equal to default still reflects" (Attribute reflection group) — verifies the Computed forceNotify path. Each test was confirmed to fail with its corresponding fix disabled. FakeElement.setAttribute / removeAttribute now call attributeChanged() on Props, mirroring real custom elements' attributeChangedCallback path. Without this, source="attribute" writes never reach Prop#set and the removeAttribute tests would silently pass even with the fix disabled. The ignoredAttributes guard inside attributeChanged prevents reflection round-trips, so Prop's existing inline reflection still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The forceNotify path is a 2-line branch inside the dedupe-bail's `if`, not a restructure of both branches. Behavior is identical: subscribers for a no-op write with force=true receive (currentValue, currentValue) either way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
e6da2b6 to
7e07a9a
Compare
Both el.prop = undefined and el.removeAttribute(name) now revert the prop to its default *and* clear any previously-reflected attribute. Reactive defaults that re-evaluate while a stale attribute lingers also clear it. Stacked on #102. The unique #107 work is a refactor: - #reflect(element, value, source): consolidates attribute writes. source === "default" maps to a null target, so the "write if different" path naturally clears stale attributes. - #settle(element, value): single source of truth for the default → parse → convert pipeline. Used from the Computed body and from set()'s rawSignal path. - #sourceFor(value): derives "default" / "convert" / "property" source from the raw user value. Reflection is decoupled from propchange events: explicit writes reflect, but the event still follows value-change semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
568c6e2 to
62a93bf
Compare
Coalesce sync writes to one event per prop per microtask drain, so propchange / propschange handlers see settled state. Adds the per-drain propschange event and updated() callback alongside the per-prop propchange event and propChangedCallback. Stacked on #108 — its regression fixes (defaults don't reflect on mount, removeAttribute restores default, explicit-write reflects even when equal to default) are inherited. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7e07a9a to
3f9a227
Compare
Both el.prop = undefined and el.removeAttribute(name) now revert the prop to its default *and* clear any previously-reflected attribute. Reactive defaults that re-evaluate while a stale attribute lingers also clear it. Stacked on #102. The unique #107 work is a refactor: - #reflect(element, value, source): consolidates attribute writes. source === "default" maps to a null target, so the "write if different" path naturally clears stale attributes. - #settle(element, value): single source of truth for the default → parse → convert pipeline. Used from the Computed body and from set()'s rawSignal path. - #sourceFor(value): derives "default" / "convert" / "property" source from the raw user value. Reflection is decoupled from propchange events: explicit writes reflect, but the event still follows value-change semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
62a93bf to
452ff5b
Compare
#108 stops defaults from *synthesizing* an attribute on mount, but leaves any attribute that a prior user write reflected. After restoring the default (via el.prop = undefined or removeAttribute), the prop value reverts but the stale attribute lingers — same applies when a reactive default re-evaluates while a prior write's attribute is still in place. Reflect defaults to a null target instead of skipping reflection. The existing "write if different" check naturally clears the stale attribute via removeAttribute, with no separate code path. Stacked on #102. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
452ff5b to
7d264ce
Compare
Gate the reflection block in #onComputedChange on source !== "default",
and compute source dynamically in the subscriber: "default" iff the
value falls through (rawSignal undefined), else the user-write source
("convert" if the prop has convert, else "property"). Fixes the
pre-existing closure-captured-source quirk along the way.
## Root cause
The signals migration (#91) moved default resolution into a Computed
signal. Its subscriber reflects on every value change regardless of
source. Pre-signals, defaults were resolved lazily in Prop#get and
never went through Prop#set, so they never triggered reflection.
## Why it matters
A child element whose Computed-backed prop is read externally before
its own initializeFor runs has its default reflected. A subsequent
programmatic write before initializeFor lands in rawSignal but the
synthesized attribute remains. initializeFor then walks
observedAttributes, sees the synthesized default, and clobbers the
programmatic write.
Surface: <color-picker><space-picker></space-picker></color-picker>
where color-picker reads space-picker.value during its own mount
(forcing its lazy first compute), then writes space_picker.value = "lab".
Without this fix, space-picker's initializeFor re-imports the synthesized
"a98rgb" attribute and clobbers the parent's intent.
## Tests
Corrects 4 existing assertions that encoded the buggy behavior. New
coverage for the convergent restore-default path lands in #107.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-signals `Prop#get` had a `value === null` fallback that returned the default. The signals path stores user-set values in a rawSignal whose fall-through to the default only triggers on `undefined`, so a `null` left over from `removeAttribute()` settled as the actual prop value. Collapse `null → undefined` when source is `"attribute"` so the rawSignal returns to its empty state and the Computed re-engages the default fallthrough. Property writes of `null` remain a legitimate user value. Tests for this scenario land in #107. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-signals, storage started `undefined`, so the equals check at the top of `Prop#set` was always false on the first user write — regardless of whether the value matched the default. Reflection always reached the DOM. The signals Computed dedupes against its cached default-resolved value, so an explicit write equal to the prior default never fires the subscriber, and reflection silently drops. Add a `forceNotify` option to `Signal`: subscribers fire on every write, even when `equals` reports no change. The cached value still respects `equals` (no-op writes don't update it), so the existing spec.equals contract — Computed cache reflects only meaningful changes — is preserved. Drop the redundant equals guard in `Computed#compute` since the setter now decides. Use `forceNotify: true` on the prop Computeds. `#onComputedChange` gates the propchange event on actual value change to keep main's value-change semantics. Reflection runs whenever the subscriber fires (driven by user intent); the event still tracks the value. Tests for this scenario land in #107. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3f9a227 to
46c2182
Compare
Gate the reflection block in #onComputedChange on source !== "default",
and compute source dynamically in the subscriber: "default" iff the
value falls through (rawSignal undefined), else the user-write source
("convert" if the prop has convert, else "property"). Fixes the
pre-existing closure-captured-source quirk along the way.
## Root cause
The signals migration (#91) moved default resolution into a Computed
signal. Its subscriber reflects on every value change regardless of
source. Pre-signals, defaults were resolved lazily in Prop#get and
never went through Prop#set, so they never triggered reflection.
## Why it matters
A child element whose Computed-backed prop is read externally before
its own initializeFor runs has its default reflected. A subsequent
programmatic write before initializeFor lands in rawSignal but the
synthesized attribute remains. initializeFor then walks
observedAttributes, sees the synthesized default, and clobbers the
programmatic write.
Surface: <color-picker><space-picker></space-picker></color-picker>
where color-picker reads space-picker.value during its own mount
(forcing its lazy first compute), then writes space_picker.value = "lab".
Without this fix, space-picker's initializeFor re-imports the synthesized
"a98rgb" attribute and clobbers the parent's intent.
## Tests
Corrects 4 existing assertions that encoded the buggy behavior. New
coverage for the convergent restore-default path lands in #107.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-signals `Prop#get` had a `value === null` fallback that returned the default. The signals path stores user-set values in a rawSignal whose fall-through to the default only triggers on `undefined`, so a `null` left over from `removeAttribute()` settled as the actual prop value. Collapse `null → undefined` when source is `"attribute"` so the rawSignal returns to its empty state and the Computed re-engages the default fallthrough. Property writes of `null` remain a legitimate user value. Tests for this scenario land in #107. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-signals, storage started `undefined`, so the equals check at the top of `Prop#set` was always false on the first user write — regardless of whether the value matched the default. Reflection always reached the DOM. The signals Computed dedupes against its cached default-resolved value, so an explicit write equal to the prior default never fires the subscriber, and reflection silently drops. Add a `forceNotify` option to `Signal`: subscribers fire on every write, even when `equals` reports no change. The cached value still respects `equals` (no-op writes don't update it), so the existing spec.equals contract — Computed cache reflects only meaningful changes — is preserved. Drop the redundant equals guard in `Computed#compute` since the setter now decides. Use `forceNotify: true` on the prop Computeds. `#onComputedChange` gates the propchange event on actual value change to keep main's value-change semantics. Reflection runs whenever the subscriber fires (driven by user intent); the event still tracks the value. Tests for this scenario land in #107. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Builds on #108 (regression fixes) to add the clear-stale-attribute behavior for reflected props. After this PR,
el.prop = undefinedandel.removeAttribute(name)converge — both revert the prop to the default and clear any previously-reflected attribute.Behavior
el.bar = X(where X ≠ default)"X""X"el.bar = Xwhere X = default"X""X"el.bar = undefinedafter a prior writeel.removeAttribute("bar")after a prior writeThe "stays X" rows are a latent bug present even on pre-signals (the old
applyChangehelper's?? element.getAttribute(...)treats explicitnullas "use fallback" and silently leaks a stale attribute). Not technically a regression — but worth fixing alongside the convergent-restore contracts.Implementation
#reflect(element, value, source)— extends the helper Restore pre-signals defaults / reflection behavior (closes #105) #108 introduces. Mapssource === "default"to anullattribute target; the existing "write if different" logic does the rest, naturally clearing stale attributes without a separate code path.#settle(element, value)— single source of truth for the default → parse → convert pipeline. Used from the Computed body and fromProp#set's rawSignal path so the inline reflection (introduced in Restore pre-signals defaults / reflection behavior (closes #105) #108) sees the same settled value.Trade-offs
propchangeevent. Explicit writes always reflect; events follow value-change semantics. Writing the current default-resolved value reflects to the DOM but doesn't firepropchange(consistent with the renamedAssigning current default-resolved value is a no-op for eventstest).el.prop = nullis preserved as a legitimate user value (mirrors JS conventions). UseundefinedorremoveAttribute()to restore the default.Test plan
npm test -- --ci— 101/102 PASS, 0 FAIL, 1 pre-existing skip insplit()Rebase note
This branch's current diff still includes parts of #108 (the regression fixes). Once #108 lands and this branch rebases on the new main, those commits dedupe automatically. The substantive remainder is
1c8149c(data-flow encoding for stale-attribute clearing),568c6e2(#settleextraction),e3f2180(docs), and the test additions.