Skip to content

Fix setAttribute() after mount not updating reflected props (closes #98)#117

Open
DmitrySharabin wants to merge 1 commit into
rollback-signalsfrom
fix-issue-98
Open

Fix setAttribute() after mount not updating reflected props (closes #98)#117
DmitrySharabin wants to merge 1 commit into
rollback-signalsfrom
fix-issue-98

Conversation

@DmitrySharabin
Copy link
Copy Markdown
Member

Port of #109 from main. Same root cause, same fix; tests are already in place.

Per the HTML spec, customElements.define only reads Class.observedAttributes if attributeChangedCallback is non-null on the prototype at registration time. Pre-this-PR, first_constructor_static wired ACB at first instance construction — after registration — so the browser snapshotted a null ACB and never read observedAttributes. setAttribute after mount silently dropped its update.

Fix: make ACB a regular entry in the props plugin's provides so addPlugins lands it on NudeElement.prototype before any subclass calls customElements.define. The eager static observedAttributes getter on providesStatic runs defineProps() lazily. Subclasses chain via super.attributeChangedCallback(...) — same convention as connectedCallback.

Bonus: also flips "removeAttribute collapses a reflected prop to its default" green — same root cause.

Stacked on #116.

Test plan

🤖 Generated with Claude Code

// Must be on the prototype chain by the time customElements.define runs:
// the spec only reads observedAttributes if attributeChangedCallback is non-null.
// https://html.spec.whatwg.org/multipage/custom-elements.html#element-definition
attributeChangedCallback (name, oldValue, value) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when this plugin is used on an element class with its own attributeChangedCallback?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should call super.attributeChangedCallback to make it work correctly. It's a trade-off of the change.

More on this here: #110

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't work. Try it


// Reserve the cache before defineProps so any consumer that reads
// Class.observedAttributes during the install (e.g., a define-props
// hook listener) gets the in-flight list instead of recursing.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could that happen? There's nothing async here, so nothing will be executed between L88 and L89 (though I haven't checked defineProps, maybe there are async parts)

Also, this doesn't take parent classes into account at all (but maybe we're fixing that later?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could that happen? There's nothing async here

It's synchronous re-entry, not async. defineProps() synchronously fires this.$hook("define-props", env) (props/index.js:71), and hook listeners run while we're still inside the getter. A hypothetical plugin that hooks define-props and peeks at the attr list before contributing its own:

const hooks = {
  define_props (env) {
    if (this.observedAttributes.includes("foo")) { /* … */ }
  },
};

Without the reservation:

  1. Outer read → getter, cache empty, no Object.hasOwn hit.
  2. Getter calls defineProps().
  3. defineProps fires the define-props hook.
  4. The listener above reads Class.observedAttributes.
  5. Getter fires again. Cache is still empty (the assignment is after defineProps).
  6. Step 5 → step 2 → step 3 → step 4 → step 5 …
  7. 💥 stack overflow.

With the reservation, step 4's inner read hits Object.hasOwn and returns the in-flight [] immediately — same re-entry-guard shape as attachShadow in src/plugins/shadow/index.js.

Today no plugin actually hooks define-props, so the guard is purely defensive — but it's a public extension point and a third-party listener would legitimately reach for Class.observedAttributes.

doesn't take parent classes into account at all (but maybe we're fixing that later?)

Right — that's the // FIXME how to combine with existing observedAttributes? on L95. Out of scope for #117; merging derived attrs with user-declared/inherited static observedAttributes is the open question from #109.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's questionable whether an empty array is better in that case, but it's not a blocking issue

@DmitrySharabin DmitrySharabin force-pushed the props-api-polish branch 2 times, most recently from 0366bc0 to 52aa111 Compare May 15, 2026 21:41
@DmitrySharabin DmitrySharabin force-pushed the fix-issue-98 branch 2 times, most recently from 025f2b5 to 787e027 Compare May 15, 2026 21:45
@DmitrySharabin DmitrySharabin force-pushed the props-api-polish branch 2 times, most recently from a3ed3b1 to e82d409 Compare May 15, 2026 21:57
Base automatically changed from props-api-polish to rollback-signals May 15, 2026 22:00
…98)

Port of #109 from `main`. Per the HTML spec, `customElements.define`
only reads `Class.observedAttributes` if `attributeChangedCallback` is
non-null on the prototype at registration time. The previous
`first_constructor_static` hook wired ACB at *first instance
construction*, which fires after registration — so the browser
snapshotted a `null` ACB and never read `observedAttributes`.
`setAttribute` after mount silently dropped its update.

Fix: make ACB a regular entry in the props plugin's `provides`, so
`addPlugins` lands it on `NudeElement.prototype` before any subclass
calls `customElements.define`. Subclasses chain via super, matching
the convention used for `connectedCallback` / `disconnectedCallback`.

The eager static `observedAttributes` getter on `providesStatic` now
runs `defineProps()` lazily — that's the registration-time path. The
`setup` hook is gated by `!Object.hasOwn(this, observedAttributes)`
so it doesn't re-install when the static getter has already done so.

Bonus: this also flips "removeAttribute collapses a reflected prop to
its default" green — same root cause.

Baseline: 91 → 93 pass / 7 → 5 fail / 4 skip.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants