Batch propchange events on a microtask drain#102
Batch propchange events on a microtask drain#102DmitrySharabin wants to merge 4 commits intofix-105-prfrom
Conversation
✅ Deploy Preview for nude-element ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
What if we keep |
5aa7ac7 to
72e605d
Compare
The real picture is not that bad, actually. The visible Initially, the actual driver of the PR wasn't #51; it was that class MyComponent extends NudeElement {
static props = { color: {...}, space: {...} };
propChangedCallback ({ name }) {
// reads BOTH; assumes they're consistent
update(this.color, this.space);
}
}
// caller does:
el.color = newColor; // handler runs — sees new color, OLD space
el.space = newSpace; // handler runs — finally settledBatching (and coalescing) is what fixes it. That's why it's also in this PR (and closes #51, but it's more of a consequence). We also unify timing: today, plain Signal props fire
Based on the investigation, I filed an issue (#104). Speaking of the changes in this PR, each class with static props gets its own Props instance with its own
Agreed. A full bag is overkill. If we don't need all, I'd do it like Lit does. If a consumer needs source semantics or attribute round-trips, etc., that's what In the codebase, internal and parsed values are effectively the same thing. The signal stores the parsed/converted value; “internal” was just our naming for it. There's no separate raw form to expose. Tracking both was over-engineering. If you like, I can split the PR into 2 or 3 separate ones. For example,
Alternatively, we can have everything in one shot. 😅 |
addEventListener can't dedupe an inline arrow wrapper, so subclasses (or any re-attachment) would multi-fire updated(). Pass this.updated directly so the DOM spec dedupes by (type, listener) triple. The handler now receives the event; consumers read e.changedProps. Mirrors propChangedCallback's stable- reference attachment. Addresses review feedback on #102.
The try/finally defended against a single #drainFor throwing — but EventTarget already isolates listener throws (DOM reports to window.onerror; Node reschedules the rethrow), so the only real-world failure mode would be dispatchEvent itself throwing, which doesn't happen with normal usage. The covering test had to monkey-patch dispatchEvent and suppress process uncaughtException to exercise it. #drain becomes a plain for-loop. Drop the artificial test along with it. Addresses review feedback on #102 (over-engineering).
Reflection-in-drain ('share one settled-state pass') was a perf shape — N
synchronous property writes produced 1 attribute write instead of N. Handlers
saw settled DOM either way, because the drain runs after sync writes either
finish or schedule a microtask. No consumer in nude-element or color-elements
does multi-write storms, so the perf benefit is theoretical.
Restore main's inline reflection: Prop.set reflects on property-source writes
when prop.toAttribute is set; #onComputedChange reflects on every settled
Computed value. The drain just dispatches.
Drop the obsolete 'Sync writes produce a single attribute write' subtest and
rename the parent describe blocks to remove the now-misleading 'deferral'
language. Update README's cycle-ordering note to reflect that reflection is
synchronous with the write, not part of the drain.
Addresses review feedback on #102 (over-engineering).
The held-back branch — guarding against payloads queued before prop.initialized flips — was unreachable. By the time #drainFor runs (from Props.initializeFor's last line, from connected(), or from the microtask drain), every prop has already had prop.initializeFor called, so prop.initialized is always true. Verified empirically: replaced the branch with throw, ran the full htest suite — 0 hits across 96 passing tests covering every propchange/coalescing/ init path. The 'remaining' Map merge after dispatch was the matching dead code; it goes too. Addresses review feedback on #102 (over-engineering).
Drop the {name, prop, detail} payload wrapper from propsupdate's changedProps
Map. Values are now just the previous value of each changed prop; consumers
read the current value via this[name].
The per-prop propchange event keeps its existing {name, prop, detail} shape —
color-elements consumers depend on it. Only the bulk callback layer changes.
Addresses review feedback on #102.
But that's exactly what #51 is about!
They do separate things though. I can see use cases where you might want all the info, but they're not the majority use case. But since we already have the detailed objects, it's also cheap to construct that map as well. So I'm leaning towards a signature like OTOH we can always ship with the first argument only, and add the second if use cases come up that need it. Yes, let's do that actually. Simplicity first!
I thought there were cases where we stored one value and the getter returned another but I could be misremembering.
I think splitting 1 into a separate PR makes sense, if it's an independent fix. 2 and 3 seem more intertwined. I still think Thanks re:subclasses. I agree we can tackle that separately. |
addEventListener can't dedupe an inline arrow wrapper, so subclasses (or any re-attachment) would multi-fire updated(). Pass this.updated directly so the DOM spec dedupes by (type, listener) triple. The handler now receives the event; consumers read e.changedProps. Mirrors propChangedCallback's stable- reference attachment. Addresses review feedback on #102.
The try/finally defended against a single #drainFor throwing — but EventTarget already isolates listener throws (DOM reports to window.onerror; Node reschedules the rethrow), so the only real-world failure mode would be dispatchEvent itself throwing, which doesn't happen with normal usage. The covering test had to monkey-patch dispatchEvent and suppress process uncaughtException to exercise it. #drain becomes a plain for-loop. Drop the artificial test along with it. Addresses review feedback on #102 (over-engineering).
Reflection-in-drain ('share one settled-state pass') was a perf shape — N
synchronous property writes produced 1 attribute write instead of N. Handlers
saw settled DOM either way, because the drain runs after sync writes either
finish or schedule a microtask. No consumer in nude-element or color-elements
does multi-write storms, so the perf benefit is theoretical.
Restore main's inline reflection: Prop.set reflects on property-source writes
when prop.toAttribute is set; #onComputedChange reflects on every settled
Computed value. The drain just dispatches.
Drop the obsolete 'Sync writes produce a single attribute write' subtest and
rename the parent describe blocks to remove the now-misleading 'deferral'
language. Update README's cycle-ordering note to reflect that reflection is
synchronous with the write, not part of the drain.
Addresses review feedback on #102 (over-engineering).
06c10f8 to
ec16bcb
Compare
The held-back branch — guarding against payloads queued before prop.initialized flips — was unreachable. By the time #drainFor runs (from Props.initializeFor's last line, from connected(), or from the microtask drain), every prop has already had prop.initializeFor called, so prop.initialized is always true. Verified empirically: replaced the branch with throw, ran the full htest suite — 0 hits across 96 passing tests covering every propchange/coalescing/ init path. The 'remaining' Map merge after dispatch was the matching dead code; it goes too. Addresses review feedback on #102 (over-engineering).
Drop the {name, prop, detail} payload wrapper from propsupdate's changedProps
Map. Values are now just the previous value of each changed prop; consumers
read the current value via this[name].
The per-prop propchange event keeps its existing {name, prop, detail} shape —
color-elements consumers depend on it. Only the bulk callback layer changes.
Addresses review feedback on #102.
| // `onprops.constructed` attaches listeners *after* `props.constructed`'s | ||
| // synchronous drain — without this re-fire, late-bound `on*` handlers | ||
| // miss the initial dispatch. Duplication for pre-connect listeners is | ||
| // the tradeoff. |
There was a problem hiding this comment.
The comment was opaque — fair. Updated in to lead with a “don't remove” warning and describe the timing concretely:
// Don't remove: re-fires initial propchange for on*= attribute handlers,
// which onprops attaches *after* the initial dispatch. Without this,
// shortcut handlers declared in HTML never see the initial event.
// Pre-connect imperative listeners receive the event twice.Background: this re-fire was actually deleted in an earlier session, which broke <x-foo onfoochange="…"> handlers in color-elements. The previous comment was meant to deter future deletion but didn't communicate that clearly. Does this read better?
|
Btw I think Claude didn't reply to this at all:
It just justified its current solution (and I agree wrt the extent of changes). But I still want to know whether there's an error in my reasoning here. |
The held-back branch — guarding against payloads queued before prop.initialized flips — was unreachable. By the time #drainFor runs (from Props.initializeFor's last line, from connected(), or from the microtask drain), every prop has already had prop.initializeFor called, so prop.initialized is always true. Verified empirically: replaced the branch with throw, ran the full htest suite — 0 hits across 96 passing tests covering every propchange/coalescing/ init path. The 'remaining' Map merge after dispatch was the matching dead code; it goes too. Addresses review feedback on #102 (over-engineering).
418f61e to
ec7c875
Compare
Drop the {name, prop, detail} payload wrapper from propsupdate's changedProps
Map. Values are now just the previous value of each changed prop; consumers
read the current value via this[name].
The per-prop propchange event keeps its existing {name, prop, detail} shape —
color-elements consumers depend on it. Only the bulk callback layer changes.
Addresses review feedback on #102.
addEventListener can't dedupe an inline arrow wrapper, so subclasses (or any re-attachment) would multi-fire updated(). Pass this.updated directly so the DOM spec dedupes by (type, listener) triple. The handler now receives the event; consumers read e.changedProps. Mirrors propChangedCallback's stable- reference attachment. Addresses review feedback on #102.
The try/finally defended against a single #drainFor throwing — but EventTarget already isolates listener throws (DOM reports to window.onerror; Node reschedules the rethrow), so the only real-world failure mode would be dispatchEvent itself throwing, which doesn't happen with normal usage. The covering test had to monkey-patch dispatchEvent and suppress process uncaughtException to exercise it. #drain becomes a plain for-loop. Drop the artificial test along with it. Addresses review feedback on #102 (over-engineering).
Reflection-in-drain ('share one settled-state pass') was a perf shape — N
synchronous property writes produced 1 attribute write instead of N. Handlers
saw settled DOM either way, because the drain runs after sync writes either
finish or schedule a microtask. No consumer in nude-element or color-elements
does multi-write storms, so the perf benefit is theoretical.
Restore main's inline reflection: Prop.set reflects on property-source writes
when prop.toAttribute is set; #onComputedChange reflects on every settled
Computed value. The drain just dispatches.
Drop the obsolete 'Sync writes produce a single attribute write' subtest and
rename the parent describe blocks to remove the now-misleading 'deferral'
language. Update README's cycle-ordering note to reflect that reflection is
synchronous with the write, not part of the drain.
Addresses review feedback on #102 (over-engineering).
The held-back branch — guarding against payloads queued before prop.initialized flips — was unreachable. By the time #drainFor runs (from Props.initializeFor's last line, from connected(), or from the microtask drain), every prop has already had prop.initializeFor called, so prop.initialized is always true. Verified empirically: replaced the branch with throw, ran the full htest suite — 0 hits across 96 passing tests covering every propchange/coalescing/ init path. The 'remaining' Map merge after dispatch was the matching dead code; it goes too. Addresses review feedback on #102 (over-engineering).
Drop the {name, prop, detail} payload wrapper from propsupdate's changedProps
Map. Values are now just the previous value of each changed prop; consumers
read the current value via this[name].
The per-prop propchange event keeps its existing {name, prop, detail} shape —
color-elements consumers depend on it. Only the bulk callback layer changes.
Addresses review feedback on #102.
00bcd97 to
e6da2b6
Compare
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.
Pre-signals, storage started `undefined`, so `equals(parsedValue, oldInternalValue)` was always false on the first user write — regardless of whether the value matched the default. Reflection always reached the DOM. The Computed signal caches its default-resolved value, so `equals(default, default)` dedupes and the subscriber silently drops the call: an explicit write equal to the prior default never reflects. Extract a small `#reflect` helper (used by `#onComputedChange` already) and call it inline from `Prop#set`'s `rawSignal` branch on user-source property writes. The settled value is computed without reading `signal.value` synchronously — that would notify the Computed's subscribers mid-write and break the drain-coalescing PR #102 establishes. `source === "attribute"` writes don't need inline reflection (the user just set the attribute via `setAttribute`); `parsedValue === undefined` writes don't either (re-engaging the default leaves the prior reflected attribute in place — that latent bug is fixed in #107). Tests for this scenario land in #107. 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>
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>
#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>
3a73e4f to
ed504b7
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>
All tests use FakeElement.with + apply / flush directly; from() and its recordEvents helper had no remaining callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Summary
propchangehandlers were observing intermediate state during multi-prop synchronous writes. Microtask-batch dispatch so handlers always see settled state, plus aconnectedlifecycle hook that drains queued events on (re)connect.Closes #51, closes #100.
Why
Microtask coalescing collapses sync writes to one dispatch per prop with the settled value, and unifies timing across plain Signal and Computed-backed props.
API
propchangepropChangedCallback(event)propschangeupdated(event)propschangefires once at the end of every drain, after every per-proppropchange.event.changedPropsisMap<name, oldValue>; read the current value viathis[name].Implementation
value/sourcewin;oldValueandoldAttributeValuepin to the first write so the payload spans the full first→last delta.prop.equalsskips coalesced no-ops (el.foo = X; el.foo = oldValue).Prop.setand#onComputedChange— not deferred to the drain.connected(element)drains the per-element dispatch queue on (re)connect (closespropchangeevents queued while disconnected are not dispatched on reconnect #100).updated()listener uses a stable method reference, so subclass re-attachment dedupes per the DOM spec.Test plan
npm test -- --ci— 99/100 PASS, 1 pre-existing skip, 0 FAILFollow-ups
static propsnot inherited from a parent class. Latent; no consumer hits it today.setAttribute()after mount does not update the property on plainreflectprops #98 —setAttributeafter mount silently fails. Once fixed, the!isConnectedguard inProps#attributeChangedcomes out.