Unify default handling under Computed-backed pattern#97
Merged
DmitrySharabin merged 2 commits intosignals-propsfrom Apr 27, 2026
Merged
Unify default handling under Computed-backed pattern#97DmitrySharabin merged 2 commits intosignals-propsfrom
DmitrySharabin merged 2 commits intosignals-propsfrom
Conversation
Follow-up to #91. Fixes the review bugs (1a/1b/2/3, sever-on-write / restore-on-undefined, parse-on-default coercion) by routing every prop with a default or a convert through the same raw Signal + Computed wrapper. The Computed body resolves defaults, applies parse, and runs convert in one place. Synthetic default<Name> props are gone. Incidentally fixes the pre-existing items A, B, and D from the #91 review (defaults reflect on mount, no redundant propchange when assigning the default-resolved value, defaults pass through convert). See PR description for the full breakdown and the change.source behavior delta. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
VerificationRan each of the scenarios from the original review against this branch. Each block sets up a fresh element, runs the actions, and shows the resulting events / state. Format mirrors the original review comment on #91. Bugs from the #91 review1a.
|
The parenthetical listed two `spec.*` keys and one bare `default` — make it consistent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LeaVerou
reviewed
Apr 27, 2026
| // Raw Signal holds the user-set value; Computed wraps it to apply | ||
| // convert and/or fall through to the default. Auto-tracks deps. | ||
| let rawSignal = new Signal(undefined); | ||
| rawSignal.equals = (a, b) => this.equals(a, b); |
Contributor
There was a problem hiding this comment.
Should we subclass instead of overridding? Or have an equals constructor option? Also creating it here seems a bit weird?
Member
Author
There was a problem hiding this comment.
Probably one of these is a better approach. I'd leave it as a follow-up improvement to the parent PR, not this one.
LeaVerou
approved these changes
Apr 27, 2026
This was referenced Apr 27, 2026
DmitrySharabin
added a commit
that referenced
this pull request
Apr 28, 2026
Convert the verification scenarios from PR #91 / #97 review comments into runnable hTest tests. They previously lived as transcripts in PR comments and relied on customElements.define, which the JS-first runner can't evaluate. - New test/util/FakeElement.js: minimal EventTarget-based mock implementing only the DOM surface Prop / Props actually touch (props, ignoredAttributes, isConnected, the four *Attribute methods). Exposes FakeElement.from(props, actions) → { Class, el, events } that builds, mounts, runs actions with a microtask flush between each, and returns the harness. - test/Prop.js: fix stale import paths (../src/props/... → ../src/plugins/props/util/...). Rework the Defaults subgroup for the post-#97 architecture (defaultProp now sugars into a function that reads this[name]; prop.default is never a Prop instance). Un-skip the previously- open precedence cases (default wins over defaultProp). Add a new top-level Runtime behavior group with three sub-groups exercising propchange events, final prop values, and attribute reflection — covering scenarios 1a, 1b, 2, 3, 4, 5a, A, B, D, sever/restore, parse-on-default, null preservation, null release, get() reactivity, and dynamic/conditional deps. - test/Props.js: fix import path. Drop the dead Props.add() static-method group (API removed pre-#66; tests were tracking a function that no longer exists). Fix the existing Props(Class) call sites to pass Class.props explicitly, since the constructor no longer auto-reads it. Add a Runtime behavior group with the pre-mount setAttribute scenario (5b) and skipped placeholders for issue #98 (post-mount setAttribute) and case C from PR #91 (queued events lost across disconnect/reconnect). - test/split.js: fix import path. Mark the Parens + quotes case as skipped — it asserted single-quote ignore-pair behaviour, but defaultPairs only enables " by default (single quotes are commented out). - package.json / package-lock.json: bump htest.dev 0.0.22 → 0.0.24. Suite: 68 / 71 PASS, 3 SKIP, 0 FAIL. Verified each new test is genuinely sensitive (passes with the real expected value AND fails when the expected is mutated) and audited for redundancy (zero tests that don't add unique branch coverage). Note: Prop / Props use EventTarget and CustomEvent as Node globals, which require Node ≥19. The system Node on the development machine is 16, so `npm test` errors at import; the suite was run under Node 22 via `PATH=~/.nvm/versions/node/v22.14.0/bin:$PATH npx htest ./test`. An `engines` field in package.json may be worth adding separately.
DmitrySharabin
added a commit
that referenced
this pull request
Apr 28, 2026
* Unify default handling under Computed-backed pattern Follow-up to #91. Fixes the review bugs (1a/1b/2/3, sever-on-write / restore-on-undefined, parse-on-default coercion) by routing every prop with a default or a convert through the same raw Signal + Computed wrapper. The Computed body resolves defaults, applies parse, and runs convert in one place. Synthetic default<Name> props are gone. Incidentally fixes the pre-existing items A, B, and D from the #91 review (defaults reflect on mount, no redundant propchange when assigning the default-resolved value, defaults pass through convert). See PR description for the full breakdown and the change.source behavior delta. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DmitrySharabin
added a commit
that referenced
this pull request
Apr 28, 2026
Convert the verification scenarios from PR #91 / #97 review comments into runnable hTest tests. They previously lived as transcripts in PR comments and relied on customElements.define, which the JS-first runner can't evaluate. - New test/util/FakeElement.js: minimal EventTarget-based mock implementing only the DOM surface Prop / Props actually touch (props, ignoredAttributes, isConnected, the four *Attribute methods). Exposes FakeElement.from(props, actions) → { Class, el, events } that builds, mounts, runs actions with a microtask flush between each, and returns the harness. - test/Prop.js: fix stale import paths (../src/props/... → ../src/plugins/props/util/...). Rework the Defaults subgroup for the post-#97 architecture (defaultProp now sugars into a function that reads this[name]; prop.default is never a Prop instance). Un-skip the previously- open precedence cases (default wins over defaultProp). Add a new top-level Runtime behavior group with three sub-groups exercising propchange events, final prop values, and attribute reflection — covering scenarios 1a, 1b, 2, 3, 4, 5a, A, B, D, sever/restore, parse-on-default, null preservation, null release, get() reactivity, and dynamic/conditional deps. - test/Props.js: fix import path. Drop the dead Props.add() static-method group (API removed pre-#66; tests were tracking a function that no longer exists). Fix the existing Props(Class) call sites to pass Class.props explicitly, since the constructor no longer auto-reads it. Add a Runtime behavior group with the pre-mount setAttribute scenario (5b) and skipped placeholders for issue #98 (post-mount setAttribute) and case C from PR #91 (queued events lost across disconnect/reconnect). - test/split.js: fix import path. Mark the Parens + quotes case as skipped — it asserted single-quote ignore-pair behaviour, but defaultPairs only enables " by default (single quotes are commented out). - package.json / package-lock.json: bump htest.dev 0.0.22 → 0.0.24. Suite: 68 / 71 PASS, 3 SKIP, 0 FAIL. Verified each new test is genuinely sensitive (passes with the real expected value AND fails when the expected is mutated) and audited for redundancy (zero tests that don't add unique branch coverage). Note: Prop / Props use EventTarget and CustomEvent as Node globals, which require Node ≥19. The system Node on the development machine is 16, so `npm test` errors at import; the suite was run under Node 22 via `PATH=~/.nvm/versions/node/v22.14.0/bin:$PATH npx htest ./test`. An `engines` field in package.json may be worth adding separately.
DmitrySharabin
added a commit
that referenced
this pull request
Apr 28, 2026
Convert the verification scenarios from PR #91 / #97 review comments into runnable hTest tests. They previously lived as transcripts in PR comments and relied on customElements.define, which the JS-first runner can't evaluate. - New test/util/FakeElement.js: minimal EventTarget-based mock implementing only the DOM surface Prop / Props actually touch (props, ignoredAttributes, isConnected, the four *Attribute methods). Exposes FakeElement.from(props, actions) → { Class, el, events } that builds, mounts, runs actions with a microtask flush between each, and returns the harness. - test/Prop.js: tighten the Defaults run function — drop the now-unreachable `instanceof Prop` unwrap branch (post-#97, prop.default is never a Prop instance) in favour of evaluating defaultProp's `function () { return this[propName] }` sugar against a `{ bar: "bar" }` stub. Un-skip the previously-open precedence cases (Value and prop, Function and prop) — the `// ??` comments dated from when default vs defaultProp precedence was undecided; settled in code as "explicit default wins". Add a top-level Runtime behavior group with three sub-groups exercising propchange events, final prop values, and attribute reflection — covering scenarios 1a, 1b, 2, 3, 4, 5a, A, B, D, sever/restore, parse-on-default, null preservation, null release, get() reactivity, and dynamic/conditional deps. - test/Props.js: add a Runtime behavior group with the pre-mount setAttribute scenario (5b) and skipped placeholders for issue #98 (post-mount setAttribute) and case C from PR #91 (queued events lost across disconnect/reconnect). Suite: 78 / 81 PASS, 3 SKIP, 0 FAIL. Verified each new test is genuinely sensitive (passes with the real expected value AND fails when the expected is mutated) and audited for redundancy (zero tests that don't add unique branch coverage).
DmitrySharabin
added a commit
that referenced
this pull request
Apr 28, 2026
Convert the verification scenarios from PR #91 / #97 review comments into runnable hTest tests. They previously lived as transcripts in PR comments and relied on customElements.define, which the JS-first runner can't evaluate. - New test/util/FakeElement.js: minimal EventTarget-based mock implementing only the DOM surface Prop / Props actually touch (props, ignoredAttributes, isConnected, the four *Attribute methods). Exposes FakeElement.from(props, actions) → { Class, el, events } that builds, mounts, runs actions with a microtask flush between each, and returns the harness. - test/Prop.js: tighten the Defaults run function — drop the now-unreachable `instanceof Prop` unwrap branch (post-#97, prop.default is never a Prop instance) in favour of evaluating defaultProp's `function () { return this[propName] }` sugar against a `{ bar: "bar" }` stub. Un-skip the previously-open precedence cases (Value and prop, Function and prop) — the `// ??` comments dated from when default vs defaultProp precedence was undecided; settled in code as "explicit default wins". Add a top-level Runtime behavior group with three sub-groups exercising propchange events, final prop values, and attribute reflection — covering scenarios 1a, 1b, 2, 3, 4, 5a, A, B, D, sever/restore, parse-on-default, null preservation, null release, get() reactivity, and dynamic/conditional deps. - test/Props.js: add a Runtime behavior group with the pre-mount setAttribute scenario (5b) and skipped placeholders for issue #98 (post-mount setAttribute) and case C from PR #91 (queued events lost across disconnect/reconnect). Suite: 78 / 81 PASS, 3 SKIP, 0 FAIL. Verified each new test is genuinely sensitive (passes with the real expected value AND fails when the expected is mutated) and audited for redundancy (zero tests that don't add unique branch coverage).
DmitrySharabin
added a commit
that referenced
this pull request
Apr 28, 2026
* Add minimal signals implementation with auto-tracking Signal: reactive value container with .value getter/setter, subscribe(), EventTarget (change events), and overridable equals(). Computed: lazy, auto-tracking signal — intercepts Signal.value reads during compute to discover deps at runtime. Re-tracks each recomputation so dynamic/conditional dependencies work correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Use signals for props reactivity, removing static dep inference (#79) Replace regex-based inferDependencies() and manual dependency cascade with signals-based auto-tracking: - Prop: add getSignal(element) to lazily create Signal (plain props) or Computed (spec.get props) per element. Reads go through signal.value for auto-tracking; writes update signal.value to propagate to dependents. Remove: dependencies field, dependsOn(), update(). - Props: remove dependents graph and updateDependents()/topological sort. propChanged() now only fires events — signals handle propagation. - util.js: remove inferDependencies() and sortObject() (no longer needed). - test/Prop.js: remove dependency inference tests, update defaults test for new computed-prop-based function defaults. Fixes #79 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix glitch-free batching, tracking leak, and cycle detection in signals Replace immediate synchronous recomputation in Computed dep subscribers with microtask-batched dirty scheduling. This prevents diamond dependency graphs (A→B, A→C, B+C→D) from computing D twice with stale data. - Add dirtyComputeds Set + queueMicrotask flush with cascade support - Cap flush iterations at 100 to detect circular dependencies - Suspend tracking context during internal super.value reads - Add recomputeIfDirty() for the flush scheduler Fixes #91 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix convert reactivity, dual-write, and computed subscriber issues in Prop - Restore convert reactivity via two-signal architecture: raw Signal holds pre-convert value, Computed applies convert and auto-tracks dependencies so convert re-runs when referenced props change - Fix dual-write by splitting set() into clean paths: convert props delegate to signal subscriber; plain props do signal→cache→reflect→events - Add shared #onComputedChange subscriber with attribute reflection support for both spec.get and spec.convert Computeds - Remove stale dependencies/additionalDependencies from README docs - Add explanatory comment for always-creating computed props for fn defaults Fixes #91 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Unify default handling under Computed-backed pattern (#97) * Unify default handling under Computed-backed pattern Follow-up to #91. Fixes the review bugs (1a/1b/2/3, sever-on-write / restore-on-undefined, parse-on-default coercion) by routing every prop with a default or a convert through the same raw Signal + Computed wrapper. The Computed body resolves defaults, applies parse, and runs convert in one place. Synthetic default<Name> props are gone. Incidentally fixes the pre-existing items A, B, and D from the #91 review (defaults reflect on mount, no redundant propchange when assigning the default-resolved value, defaults pass through convert). See PR description for the full breakdown and the change.source behavior delta. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add tests for Prop / Props runtime behavior (#99) Convert the verification scenarios from PR #91 / #97 review comments into runnable hTest tests. They previously lived as transcripts in PR comments and relied on customElements.define, which the JS-first runner can't evaluate. - New test/util/FakeElement.js: minimal EventTarget-based mock implementing only the DOM surface Prop / Props actually touch (props, ignoredAttributes, isConnected, the four *Attribute methods). Exposes FakeElement.from(props, actions) → { Class, el, events } that builds, mounts, runs actions with a microtask flush between each, and returns the harness. - test/Prop.js: tighten the Defaults run function — drop the now-unreachable `instanceof Prop` unwrap branch (post-#97, prop.default is never a Prop instance) in favour of evaluating defaultProp's `function () { return this[propName] }` sugar against a `{ bar: "bar" }` stub. Un-skip the previously-open precedence cases (Value and prop, Function and prop) — the `// ??` comments dated from when default vs defaultProp precedence was undecided; settled in code as "explicit default wins". Add a top-level Runtime behavior group with three sub-groups exercising propchange events, final prop values, and attribute reflection — covering scenarios 1a, 1b, 2, 3, 4, 5a, A, B, D, sever/restore, parse-on-default, null preservation, null release, get() reactivity, and dynamic/conditional deps. - test/Props.js: add a Runtime behavior group with the pre-mount setAttribute scenario (5b) and skipped placeholders for issue #98 (post-mount setAttribute) and case C from PR #91 (queued events lost across disconnect/reconnect). Suite: 78 / 81 PASS, 3 SKIP, 0 FAIL. Verified each new test is genuinely sensitive (passes with the real expected value AND fails when the expected is mutated) and audited for redundancy (zero tests that don't add unique branch coverage). * Pass signal equality at construction via { equals } option (#101) Follow-up to #91. Resolves the design discussion at #91 (comment) (Option 2): replace the post-construction `signal.equals = …` monkey-patches in Prop.js with a constructor option. - src/signals.js: Signal constructor accepts `{ equals }` (applied as an instance override). Computed forwards options to `super(undefined, options)`. - src/plugins/props/util/Prop.js: getSignal now passes `{ equals: (a, b) => this.equals(a, b) }` to all four Signal/Computed construction sites; the two post-hoc patches go away. Tests: - test/signals.js (new): direct unit tests. With `equals: () => true`, no subscriber notification fires from a write — discriminates against default `===` (which would notify). - test/Prop.js: integration test "spec.equals: tolerated dep change leaves cached value" in the Final value group. A spec.get prop with tolerance equals; a within-tolerance dep change leaves the Computed's cached value untouched. Verified to fail when the option is removed from any of the four Prop.js construction sites. An earlier event-list version of this test was abandoned because hTest's default array check at check.js#L61 prefix-matches actual against expect, silently tolerating extra events. Suite: 81 / 84 PASS (3 pre-existing skips), 0 FAIL. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Dmitry Sharabin <dmitrysharabin@gmail.com>
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
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.
Summary
Follow-up to #91. Fixes the bugs in the review and unifies default handling under the Computed-backed pattern.
Bugs fixed (review scenarios from #91)
defaultPropsource-change propagationdefaultBar)defaultPropparse()(andconvert, if specified)Architectural change
getSignalcollapses to two default-handling shapes:spec.get): plain Computed.Synthetic
default<Name>props are gone from the registry.Pre-existing items also fixed (incidentally)
reflect: truenow reflect to the attribute on mount — covers bothconvert+default+reflect(the case in the original review) and plaindefault+reflect.spec.convert.Behavior delta worth flagging
change.sourcefor user writes on Computed-backed props is now"default"/"convert"(the construction-time path), not"property"/"attribute". Pre-#91, function-default props were plain Signal and reported"property"/"attribute". This unifies their behavior withspec.convert, which already reported"convert"regardless of trigger. No source consumers grep'd in tree.Out of scope
Item C from the #91 review (queued events lost on disconnect/reconnect) — not touched by this PR.