Fix early-return conflation in Prop.set (oldValue + propchange skip)

[DmitrySharabin]

Summary

set() in Prop.js had a single if (equals(parsedValue, oldValue)) return; that skipped three concerns at once — attribute reflection, propchange dispatch, and the storage write. Because element.props[name] was never populated with a literal default, oldValue was undefined on the first write, masking the bug behind a false equality.

This PR splits those concerns:

• set() resolves the default lazily when element.props[name] is undefined and the prop has a literal default. That gives the equality check and propchange.detail.oldValue the same value the getter would return — without writing into element.props[name], which stays as the "explicit value" indicator. Function defaults stay lazy (their original call-site timing); defaultProp resolves through the dependency cascade.
• set() runs the reflection branch before the equality check, preserving the "explicit equal write reflects" contract (Roll back signals and batching and reimplement them #113). The inner attribute-mismatch gate still skips redundant DOM writes.

New regression test pins propchange.detail.oldValue to the resolved default on first write — the only test in the suite asserting on detail.oldValue. The pre-existing Assigning current default-resolved value is a no-op baseline was also tightened to use a strict count delta (events.length - before === 0) rather than relying on hTest's prefix-based array equality, which silently allowed the bug.

Uncovered while implementing propchange coalescing — the latent bug surfaced once consumers started reading detail.oldValue on coalesced events.

Evolution

The first version of this PR cached resolved literal defaults into element.props[name] during initializeFor. @LeaVerou flagged that this collapses two distinct states ("explicit equal write" vs "at default") into one. The current version resolves lazily in set() instead, preserving the distinction.

Test plan

• npm test — both new and pre-existing target tests pass with the fix
• Both target tests fail correctly when Prop.js is reverted (RED state verified)
• Explicit write equal to the default still reflects to the attribute (the Roll back signals and batching and reimplement them #113 contract) still passes
• element.props[name] remains undefined for "at default" state — Lea's invariant preserved

🤖 Generated with Claude Code

[netlify]

✅ Deploy Preview for nude-element ready!

Name	Link
🔨 Latest commit	bd3dc85
🔍 Latest deploy log	https://app.netlify.com/projects/nude-element/deploys/6a0affb53c93ed00085f7225
😎 Deploy Preview	https://deploy-preview-122--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.

[LeaVerou]

Because element.props[name] was never populated with a literal default, oldValue was undefined on the first write, masking the bug behind a false equality.

Was? Wait, is it now populated with the default? That's wrong. Then how can we tell the difference between an explicitly written value that happens to be the same as the default vs a prop that's just set to its default value?

Caching resolved values makes sense, but we should still have access to the actual, real internal value. They shouldn't clobber it.

[LeaVerou]

I think caching resolved values is the right way to go, we just need to cache them separately. With the recent change, it now resolves old values based on current data. But this may have changed, e.g. if the default value is dynamic. When we're monitoring for changes, we're usually interested in changes in resolved values. I'm not sure we're ever interested in changes in internal values, but this will become clearer with use cases.

Also, we currently have this class-wide Props object that we pass element to as an argument, which is kind of a weird hybrid OOP-procedural architecture. Perhaps we need to have an ElementProps object that is created separately per element (and then element.props would be that object). Then we can cache values there, in a separate data structure.

Another thing to keep in mind is that down the line we want to split this plugin into separate plugins:

• One for reflection
• One for defaults
• Perhaps one for convert
• Perhaps one for computed props
• etc

[DmitrySharabin]

Took your feedback and reworked the design. Wanted to run it by you before implementing.

Two stores

element.props becomes a per-element ElementProps instance holding two Maps:

• #explicit — values consumers explicitly set. undefined here = "at default" (your invariant preserved).
• #cached — the value the getter last returned to consumers. This is what propchange.detail.oldValue reads.

The two stores never clobber each other. The current PR's "resolve default on the fly inside set()" branch goes away — that logic moves into Prop.get()'s cache-miss path.

Responsibility split

Object	Scope	Methods
Prop	Per-prop definition + lifecycle verbs (set / get / update / initializeFor)	Signatures unchanged — still (element, …)
Props	Per-class registry + dependency graph	Unchanged
ElementProps	Per-element runtime (the two stores)	New — see API below

API

class ElementProps {
    getExplicit(name)    getCached(name)
    hasExplicit(name)    hasCached(name)

    set(name, value)       // both stores (the Prop.set happy path)
    cache(name, value)     // cache only (after the getter resolves a default)

    reset(name)            // both stores (e.g., removeAttribute → undefined)
    invalidate(name)       // cache only (defaultProp cascade)
}

One verb per intent. Reads pair with has* as a query family.

How Prop.js changes

Location	Diff
initializeFor — "at default" guard	- else if (element.props[name] === undefined && !this.defaultProp) { + else if (!element.props.hasExplicit(name) && !this.defaultProp) {
get — explicit read (×2 sites in the resolution chain)	- let value = element.props[this.name]; + let value = element.props.getExplicit(this.name);
set / update — oldValue (semantic shift: now reads cached resolved)	- let oldValue = element.props[this.name]; + let oldValue = this.get(element);
dependsOn — "at default" check	- (this.defaultProp === prop && element.props[this.name] === undefined) + (this.defaultProp === prop && !element.props.hasExplicit(this.name))

Three structural changes — each its own diff:

set write site — undefined → reset, value → set:

- element.props[this.name] = parsedValue;
+ if (parsedValue === undefined) {
+     element.props.reset(this.name);
+ } else {
+     element.props.set(this.name, parsedValue);
+ }

set — remove the lazy resolution branch this PR added; this.get(element) above already populates the cache and oldValue reads from it:

- // Resolve the default lazily so equality and propchange.oldValue match the getter.
- if (
-     oldValue === undefined &&
-     !this.spec.get &&
-     this.default !== undefined &&
-     typeof this.default !== "function"
- ) {
-     oldValue = this.get(element);
- }

get — cache-check fast path on entry; populate-on-miss before return:

  get (element) {
+     if (element.props.hasCached(this.name)) {
+         return element.props.getCached(this.name);
+     }
+
      let value = element.props.getExplicit(this.name);
      // … existing resolution chain unchanged …
+     element.props.cache(this.name, value);
      return value;
  }

update — defaultProp cascade captures oldResolved before invalidating, threads it through:

  if (dependency === this.defaultProp) {
-     this.changed(element, { element, source: "default" });
+     let oldResolved = this.get(element);
+     element.props.invalidate(this.name);
+     this.changed(element, { element, source: "default", oldValue: oldResolved });
      return;
  }

Cache lifecycle

• Populated on every Prop.get(element) (cache-miss runs the resolution chain, then cache(name, value)), and on every successful Prop.set write.
• Invalidated explicitly only on the defaultProp cascade (invalidate) and on attribute removal (reset). For spec.get / convert cascades, the dependent's own set() overwrites the cache atomically — no separate eviction needed.
• oldValue always reads from the cache via Prop.get(element) (populates on miss). For dynamic defaults that change between reads, this captures what consumers actually saw — not a fresh re-resolution against current data.

Test pinning the dynamic-default fix

let counter = { value: 1 };
defineProps({ size: { type: Number, default: () => counter.value } });
// counter is closed-over external state; inferDependencies only matches `this.x`,
// so no defaultProp is created and no cascade fires when counter changes.

element.size;            // → 1, cache captures 1
counter.value = 2;       // no cascade — cache stays at 1
element.size = 5;        // explicit write

propchange.detail.oldValue === 1   // ← cached value consumers actually saw

Under this PR currently: oldValue === undefined (the lazy-resolution branch in set() excludes function defaults). Under the proposed design: oldValue === 1.

What this leaves alone

• Future plugin split (reflection / defaults / convert / computed) not blocked by this design.

Open question: verb migration in this PR?

The proposal above moves the storage onto ElementProps but keeps the methods (set / get / update / initializeFor) on Prop with (element, …) signatures. That half-addresses your "weird hybrid OOP-procedural" point. Wanted to give you a clear prompt to decide whether to go the rest of the way in this PR.

Call sites:

// Today + proposal as written:
prop.set(element, value, { source });
prop.get(element);
prop.update(element, dep);

// With verb migration:
element.props.set(name, value, { source });
element.props.get(name);
element.props.update(name, dep);

What changes structurally:

• Prop shrinks to a pure definition object: name, spec, type, dependencies, parse, stringify, equals, reflect config. No methods that take element.
• ElementProps owns the per-element runtime in full — storage and verbs.
• Props (class-level registry) still holds prop definitions and the dependency graph; unchanged otherwise.

What we get:

1. Full fix for the OOP-procedural concern — receiver always matches scope.
2. Call shape reads more naturally throughout the plugin internals.
3. Future plugin-split PR becomes a focused carve — no structural move on top of the semantic work.
4. Prop becomes the obvious extension point for plugin-contributed metadata, since it's pure definition.

Cost:

• Roughly 2× this PR's diff. ~80 lines of method bodies migrate from Prop to ElementProps; call sites in Props.js and Prop.getDescriptor update accordingly.
• Mechanical — no behavior change, no new abstractions. Covered by the existing test suite plus the new dynamic-default test.

Trade-off:

Total refactoring work is the same either way — the method bodies eventually move and get carved into plugin hooks. Question is whether the move lands here or in the plugin-split PR. Doing it here means this PR addresses your architecture point in full; deferring keeps this PR tightly scoped to the bug fix.

Your call. Full spec is in a local doc (lifecycle invariants, full migration table with file:line refs, complete test plan); happy to push it to the branch if useful. Does the rest of the design land where you'd want, and do you want the verb migration folded in?

[DmitrySharabin]

I think that adding ElementProps just for caching values is a bit too much. So, moving cache resolution and the corresponding methods together seems reasonable. And it seems like having everything related to element props in one place makes it easier to transform it into a plugin later (or right away, not sure). To glue the new class with the props plugin, we just need to tweak it in one place: the lazy props property that it provides to the instances should return a new instance of the ElementProps object instead of an empty object like it does now. Later, we can probably call hooks (like reflect, parse, convert, etc.) in the corresponding places (for example, when setting a value) to glue the rest of the plugins together.

So, this might be a good first step if done right.

Speaking of the API, it feels like a bit too much hustle to do simple things: we should know what method to call to store a value (either as an explicit value, resolved, or both). Having two maps might also make them out of sync (probably, not sure). I honestly don’t know what a good design might look like here.

[LeaVerou]

I think that adding ElementProps just for caching values is a bit too much. So, moving cache resolution and the corresponding methods together seems reasonable. And it seems like having everything related to element props in one place makes it easier to transform it into a plugin later (or right away, not sure). To glue the new class with the props plugin, we just need to tweak it in one place: the lazy props property that it provides to the instances should return a new instance of the ElementProps object instead of an empty object like it does now. Later, we can probably call hooks (like reflect, parse, convert, etc.) in the corresponding places (for example, when setting a value) to glue the rest of the plugins together.

So, this might be a good first step if done right.

Agreed.

Speaking of the API, it feels like a bit too much hustle to do simple things: we should know what method to call to store a value (either as an explicit value, resolved, or both). Having two maps might also make them out of sync (probably, not sure). I honestly don’t know what a good design might look like here.

Yup. I think Claude has overcomplicated this, as it often does. I suspect the reason might be that we are trying to do it as a drive-by part of another PR, that is ultimately unrelated. I suspect we'll have better results if it's done as a separate piece of work. But this.props.foo = internalValue is an invariant we should try to preserve.

And we may end up having to introduce ElementProp as well, not sure. I may experiment with it a bit.

[DmitrySharabin]

I'm going to close this in favor of #129.