Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: React overwrites functions on customElements #31689

Closed
mrginglymus opened this issue Dec 6, 2024 · 6 comments
Closed

Bug: React overwrites functions on customElements #31689

mrginglymus opened this issue Dec 6, 2024 · 6 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@mrginglymus
Copy link

React version: 19.0.0

Steps To Reproduce

  1. Define a custom element as shown:
class CEWithAttrPropertyClash extends HTMLElement {
    test(value) {
        this.dispatchEvent(new CustomEvent('test', {detail: value}))
    }
    connectedCallback() {
        this.test(true);
    }
}

customElements.define('ce-with-attr-property-clash', CEWithAttrPropertyClash);
  1. Render
render(<ce-with-attr-property-clash test />);

Link to code example: https://codesandbox.io/p/sandbox/green-resonance-kpwcqg?file=%2Fsrc%2FApp.js%3A10%2C1

The current behavior

this.test is not a function

The expected behavior

React sets attribute as an attribute and does not override function definition

The culprit appears to be this line:

Which does not check to see if the property is a settable property.

@mrginglymus mrginglymus added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Dec 6, 2024
@mrginglymus
Copy link
Author

I've added a PR (webcomponents/custom-elements-everywhere#2466) demonstraing the difference in behaviour between React, Vue, Svelte and Angular. Vue obviously requires you to specify that it is an attribute (otherwise it too assumes a prop).

@mrginglymus
Copy link
Author

I apologise for coming to this so late to the party (ie, two days after release instead of while still in beta...)

The more I look at this, the more I think that these lines need to be removed altogether:

if (name in (node: any)) {
(node: any)[name] = value;
return;
}

Whilst many tools that generate custom elements offer property bindings to observed attributes (e.g., lit) allowing for direct setting on the instance, that's not really part of the custom element spec, but an implementation detail of those frameworks.

It seems that these lines are solely there to support something that isn't part of the custom element spec.

@mrginglymus
Copy link
Author

I've had a chance to dip into some of #11347 to get an idea of the history here, and I don't want to knock any old scabs, but having upgraded from react 18 to react 19 I was disappointed to find support for my minimal hand-rolled custom elements had actually worsened.

@trusktr
Copy link

trusktr commented Dec 15, 2024

Thanks for pointing this out @mrginglymus. As we add more tests to Custom Elements Everywhere, React will fall behind with the current design, but hopefully it will catch up again.

This is the new test proposal for Custom Elements Everywhere:

And here's the new work towards testing React (thanks @mrginglymus!):

Frameworks that offer control in their template syntax (f.e. Lit, Solid.js, and others) will already pass these new tests.

@justinfagnani
Copy link

React has an ambiguity between property and attribute names in JSX, since it combines them into a single namespace. The solutions were to either disambiguate with syntax, or add a heuristic to try to disambiguate at runtime.

They chose to disambiguate at runtime, but the gambit is that they custom elements will have to be "well-behaved". In this case it means that if an element has a both a property and attribute with the same name, then that property should reflect the attribute.

I personally think that's a fine compromise for now, but it's going to need to be better documented in the web components community. Note that Preact, and I believe Svelte and Vue already work this way (though at least Vue also has syntax to disambiguate), so your elements that regressed in React 19 already likely had issues in other frameworks.

@mrginglymus
Copy link
Author

Thanks @justinfagnani - this concept of 'well behavedness' is what I was missing I think. I'm actually happier with the design of my new custom elements that are well behaved (or at the very least less misbehaved), even if i had go through a few tools to find one that had fully implemented decorators to get there ;)

Closing issue for the sake of tidiness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

3 participants