Restore pre-signals defaults / reflection behavior (closes #105) by DmitrySharabin · Pull Request #108 · nudeui/element

DmitrySharabin · 2026-05-06T09:46:04Z

Closes #105.

Restore three default-reflection contracts that the signals migration (#91) regressed. While editing the same #onComputedChange conditional, also clear a long-standing latent bug — leaving it would have left #105 partially open. The production color-elements site runs on 0.1.3 (pre-signals); these restore the contracts it relies on, so #102 can ship without regressions in its diff.

What's restored

Scenario	Pre-signals	Post-signals (origin/main)	This PR
Default reflects on mount	no	yes (regression)	no
`removeAttribute(name)` → `el.prop`	default	null (regression)	default
Explicit `el.prop = X` where `X` matches the default	reflects `"X"`	no reflection (regression)	reflects `"X"`
Reverting to default leaves a stale reflected attribute from a prior write	yes (latent)	yes	no

The 4th row is a latent bug present even pre-signals: applyChange's ?? element.getAttribute(...) made explicit null mean "fall back to current attribute," so callers had no way to express "remove this attribute" through the change object. This PR fixes both layers — #onComputedChange passes null when reverting to default (clears the stale attribute via the existing removeAttribute branch), and applyChange is tightened so callers can actually request removal.

Stats

GitHub shows +97 / −23 across 5 files. The behavior change is small; most of the diff is tests:

Category	Net lines	File(s)
Source code logic + JSDoc	~+23	Prop.js (+32), signals.js (−9)
Documentation	+2	README.md
Adjusted + new regression tests	+45	test/Prop.js
Test infrastructure	+4	test/util/FakeElement.js (`setAttribute` / `removeAttribute` now call `attributeChanged`, mirroring real custom elements)

src/signals.js is net −9 lines — no new Signal API. The redundant equals guard in Computed#compute is removed and Signal#set decides.

Each new test was confirmed to fail with its corresponding fix disabled, then pass with the fix re-enabled.

Implementation

#onComputedChange: if (this.toAttribute) { let attributeValue = source === "default" ? null : this.stringify(newValue); ... }. The existing "write only if different" check handles both directions:
- No prior attribute, target null → no-op (default doesn't synthesize).
- Prior write left an attribute, target null → removeAttribute (clears stale).
Source derivation: Compute source dynamically in the subscriber — "default" iff rawSignal.value === undefined, else the user-write source ("convert" or "property").
Prop#set: Collapse null → undefined when source === "attribute", so removeAttribute returns the rawSignal to its empty state and the Computed re-engages the default fallthrough.
Prop#set Computed-backed branch: After writing rawSignal, reflect inline for source === "property" user writes. The Computed dedupes writes equal to the cached default (e.g. el.v = 5 when default resolves to 5), so the subscriber wouldn't fire — inline reflection covers that. convert is applied manually instead of via signal.value to avoid forcing a sync recompute, which would split microtask cascade batching.
applyChange ?? → !== undefined ?: Closes the latent bug above. Now explicit null means "remove"; only an omitted attributeValue falls back to the source element's current attribute.

Why now

PR #102 currently inherits the regressions. Landing this first lets #102 rebase cleanly on a fixed main, so it ships without regressions in its diff.

Test plan

npm test -- --ci — 85/88 PASS, 3 pre-existing skips, 0 FAIL

netlify · 2026-05-06T09:46:09Z

✅ Deploy Preview for nude-element ready!

Name	Link
🔨 Latest commit	`c579032`
🔍 Latest deploy log	https://app.netlify.com/projects/nude-element/deploys/69fd9d218111020008e034b1
😎 Deploy Preview	https://deploy-preview-108--nude-element.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

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>

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>

#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>

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>

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>

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>

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>

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>

LeaVerou · 2026-05-06T18:32:57Z


 	set value (v) {
 		if (this.equals(v, this.#value)) {
+			if (this.#force) {


Given that the whole point of equals is to determine whether to notify, wouldn't it be easier to just set an equals that always returns false? Does it do anything else?

There's actually one thing that it does additionally. It gates the cache update (e.g., on derived values). When equals(new, current) is true, the setter returns early, and #value stays the same (we don't reach L84). If an author uses a tolerance-based equals (for example, (a, b) => Math.abs(a - b) < 0.1), by replacing their function with () => false, we'll break the author's logic. We already have a regression test for this in test/Prop.js#L860 in the upcoming PR.

Ok, then we should rename the option, because force as a constructor option doesn't make a lot of sense. Force what?

Yeah, that was bothering me as well. How about notifyOnEquals? It's aligned with the equals option and explicitly expresses what it's meant to.

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>

LeaVerou · 2026-05-07T20:01:39Z

Signal force option: Subscribers fire on every dep-driven recomputation, even when equals reports no change. The cache still respects equals (preserves the spec.equals contract). Reflection now tracks user intent rather than Computed dedupe; #onComputedChange gates the propchange event on actual value change.

Just realized I don't actually understand the rationale here. Why do we want to force notify even when the values are equal? It worries me we may be doing it because there is some very specific case where we need notify to fire, and Claude took the nuclear option instead…

Also fix `applyChange` to honor explicit `null` attributeValue as "remove".

DmitrySharabin mentioned this pull request May 6, 2026

Convergent restore-default for reflected props #107

Closed

2 tasks

DmitrySharabin force-pushed the fix-105-pr branch from 0e5c63b to 3178b7d Compare May 6, 2026 09:52

DmitrySharabin changed the title ~~Defaults don't reflect to attributes (closes #105)~~ Restore pre-signals defaults / reflection behavior (closes #105) May 6, 2026

DmitrySharabin force-pushed the fix-105-pr branch 3 times, most recently from dc39e61 to e7290bd Compare May 6, 2026 11:20

DmitrySharabin and others added 12 commits May 6, 2026 16:54

Remove unnecessary words

7541a94

Tweak comment

93991c7

Add default value

b35b8c3

Minimize diff

774cfa2

Minimize diff

47d8d78

Rename: forceNotify → force

4f05f3d

Minimize Signal setter further

7ed8d23

DmitrySharabin force-pushed the fix-105-pr branch from 7021bfc to 7ed8d23 Compare May 6, 2026 14:54

DmitrySharabin changed the title ~~Restore pre-signals defaults / reflection behavior (closes #105)~~ Fix default-reflection bugs (closes #105) May 6, 2026

DmitrySharabin mentioned this pull request May 6, 2026

Batch propchange events on a microtask drain #102

Open

1 task

Update comment

e664465

DmitrySharabin changed the title ~~Fix default-reflection bugs (closes #105)~~ Restore pre-signals defaults / reflection behavior (closes #105) May 6, 2026

LeaVerou reviewed May 6, 2026

View reviewed changes

Rename: force → notifyOnEquals

832858c

Replace notifyOnEquals with inline reflection in prop.set

c579032

Also fix `applyChange` to honor explicit `null` attributeValue as "remove".

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore pre-signals defaults / reflection behavior (closes #105)#108

Restore pre-signals defaults / reflection behavior (closes #105)#108
DmitrySharabin wants to merge 15 commits intomainfrom
fix-105-pr

DmitrySharabin commented May 6, 2026 •

edited

Loading

Uh oh!

netlify Bot commented May 6, 2026 •

edited

Loading

Uh oh!

LeaVerou May 6, 2026

Uh oh!

DmitrySharabin May 6, 2026 •

edited

Loading

Uh oh!

LeaVerou May 6, 2026

Uh oh!

DmitrySharabin May 7, 2026

Uh oh!

LeaVerou commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

DmitrySharabin commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's restored

Stats

Implementation

Why now

Test plan

Uh oh!

netlify Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for nude-element ready!

Uh oh!

LeaVerou May 6, 2026

Choose a reason for hiding this comment

Uh oh!

DmitrySharabin May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeaVerou May 6, 2026

Choose a reason for hiding this comment

Uh oh!

DmitrySharabin May 7, 2026

Choose a reason for hiding this comment

Uh oh!

LeaVerou commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DmitrySharabin commented May 6, 2026 •

edited

Loading

netlify Bot commented May 6, 2026 •

edited

Loading

DmitrySharabin May 6, 2026 •

edited

Loading