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

[React 19] custom element property vs. attribute behavior deviates from RFC discussion and proposal doc #29037

Open
cjpillsbury opened this issue May 9, 2024 · 17 comments
Labels

Comments

@cjpillsbury
Copy link

Summary

Hey folks! Not sure if the decisions here changed and I missed it, but it looks like the current React 19 beta impl for react props -> web component props/attrs has changed from what was originally discussed by @josepharhar. The tl;dr:

Unlike Preact, "complex values" were originally going to only be assigned to properties (if/when they exist). If the property does not exist, no value would be assigned. Instead, only functions and Symbols are omitted from setting as attributes.

Not only does this result in likely undesired behavior (e.g. anattr="[object Object]"), it can also result in runtime errors that are potentially difficult to debug, e.g.:

<my-wc iThoughtThisWasAProp={[aSymbol]}><my-wc> // This will crash

It looks like the originally planned logic could be introduced in this general vicinity. Not sure if this is a bug, a feature request, or just a discussion as to why the change in plans, but just wanted to surface for visibility, just in case. Also, happy to cobble together a codesandbox or similar if that would be helpful!

@josepharhar
Copy link
Contributor

It's been a while, and I don't remember what the thinking as around Symbols. A codesandbox would be helpful!

@cjpillsbury
Copy link
Author

The primary issue isn't Symbol per say, though an array with a Symbol value in it is particularly eggregious, since it causes an app crash. The tl;dr - in the original discussion (at least as I understood it, unless I missed something), complex values (e.g. typeof val === 'object', maybe with exceptions for null, though plausibly unnecessary) would never be set as attributes. That makes sense for consistency between (eventual) SSR/partial hydration, but also from a usefulness perspective for well-designed Web Components.

Here's a stackblitz example with a bunch of comments talking through some of the permutations:
https://stackblitz.com/edit/vitejs-vite-yrmfxm?file=src%2FApp.jsx

Happy to discuss more. It does look like inserting this logic should be narrow (per my prior comment+link), though I 💯 could be missing some larger scope concerns/considerations.

@josepharhar
Copy link
Contributor

Thanks for the example with comments. It seems like all of the problematic cases are indeed when we aren't running on the client/browser or when the custom element hasn't been upgraded yet, and you currently have to work around it by doing things like situation number 1 with example code in your comment here.

This is still a limitation of the support for setting values as object properties instead of attributes in React. I don't know if the web components ecosystem will mature enough for step 3 in my doc, but I suppose that step 2 is something that is worth working on - although I'm not familiar enough with Suspense boundaries or server rendering to figure out how to prevent rendering custom elements until they are upgraded.

I can say that one of the pieces to prevent custom elements from being rendered until they are upgraded would be to use whenDefined() within React for every custom element that is encountered. I'm also not sure how difficult this would be to implement within React's architecture.

I'm not sure I can prioritize diving into this over my other work, so help would be appreciated.

@cjpillsbury
Copy link
Author

cjpillsbury commented May 21, 2024

Cool I think I read

I propose that we render nothing for objects, arrays, and functions. I’ve heard that people are using JSON.stringify already if they really want to stringify their objects.

as part of step 1, given

Implement Preact-like support for client side rendering, but probably with some tweaks to improve the behavior.

and also since it seems like an improvement regardless of any async and/or SSR support. Not a callout, just highlighting where the misunderstanding occurred.

I'm happy to take a stab at a PR (also likely within the bounds of my other obligations). Might be able to get something going this weekend 🤞, with the tl;dr being - refactor core impl so React only ever sets properties if the value is an array or an object (this is already true for functions).

Also, tangentially related: I'm currently doing a blog post writeup on the status of all this crud and noting a few other places for potential improvements or at least discussion. Let me know if you have any gut reactions to these, think they need more broad discussion, suggestions on opening new issues, etc. etc. Here are a couple:

  1. sorting the react props before running the under the hood logic for event handler vs. prop vs. attribute setting so that the "event handler candidates" are always applied first
    i. Motivation: many react developers aren't used to having to reason about order in their JSX, but at least some event handlers may be dispatched as a result of other properties/attributes getting set or simply based on initialization crud. This will at least reduce the cognitive load for React devs
  2. Consider changing the order of checks for "event handler" vs. "there is a property with a corresponding name"
    i. Motivation: There are edge cases where a property reasonably exists that isn't for events (e.g. onto onboard, etc.). The current workaround if these accept functions (that doesn't involve refs + hooks) is for Web Component authors to add a second accessor with a different name for the same purpose. There are also cases where, like "built in" HTML elements, there are corresponding properties for event handlers e.g. an onclick property. Assuming Web Component authors are implementing these correctly, these should work fine for the event use case.

Thanks for the convo/updates. I'll be sure to link relevant issues to any PRs opened. I'll also try to peel away some time to see what kind of concerns there are on the Next.JS side of the fence.

@cjpillsbury
Copy link
Author

I can say that one of the pieces to prevent custom elements from being rendered until they are upgraded would be to use whenDefined() within React for every custom element that is encountered. I'm also not sure how difficult this would be to implement within React's architecture.

I do think this should be treated as a separate effort. Thus far I surprisingly haven't encountered this issue with basic smoke testing (surprisingly, since I'm presuming from your discussion that there's been nothing implemented to guarantee this)

@hesxenon
Copy link

hesxenon commented Jul 3, 2024

Not sure I fully understand the finer points here, but I have to say that this discussion (and the previous discussions since 2017) seem odd to me and I regret not having watched out for and participated in this sooner.

Can someone please tell me which parts of the HTML or Custom Elements specs have lead to the assumption that web component integration has to consider complex values for attributes?

As far as I understand it HTML only contains strings (duh) and web components are meant as a way to create components that are not (yet) in the HTML standard. So if they are meant as a way to extend HTML (again, a strictly serialized format) why does react consider it necessary to enable non-serializable attributes (props)?

I realize I'm very late to the party but I think this is the wrong approach as it pulls custom elements into the framework area because at this point the conflation between attributes and props carries over into an area that only specifies the former - i.e. CEs should never have complex properties, otherwise they aren't even usable without a framework.

Now this would all be fine if CE authors were aware that they're basically writing "React/Angular Custom Elements" instead of simply "Custom Elements" but it has the potential to break regular CEs since e.g. the global readonly attribute can't be properly implemented by these web components. React might set the prop, but not the attribute which could at least break intended styling.

@cjpillsbury
Copy link
Author

cjpillsbury commented Jul 17, 2024

I think I'd respectfully disagree here on a number of points, but the overarching concern here is that you're proposing that React's support of custom elements should be driven by a controversial view of how custom elements are supposed to be used and not the (lack of) official stance on how they should be used (https://html.spec.whatwg.org/dev/custom-elements.html), nor how they are actually used, fairly widely, in the ever growing web components community and set of frameworks built on web components' specifications.

Without diving in too deeply:

  1. Like native HTML elements, web components define both a declarative, HTML-based API and a JavaScript API that extends the HTMLElement.
  2. HTML elements deal with complex objects. Here is an example of of official best practices for lists that can/should be reflected between attributes and properties. Here is an example of official best practices for considerations when dealing with complex objects. I work primarily in the media space, where complex values and JavaScript are heavily used in all but the simplest of use cases of <video> and <audio>.
  3. Custom elements are allowed to and often have good use cases for dealing with complex values. Here is a fairly well-regarded, framework-agnostic discussion of some best practices around this.

There's certainly more that can be said here, but these are at least some of the high level things worth considering as a starting point.

@vageez
Copy link

vageez commented Jul 17, 2024

Not sure if I’m missing something here, but the solution that works for me is to JSON.stringify complex values that are being passed into web-components. Managing of the stringified value would need to be JSON parsed in the web component.

Not sure about Symbols tho.

@cjpillsbury
Copy link
Author

cjpillsbury commented Jul 18, 2024

@vageez yup that's certainly an option for both web component implementors and folks using/integrating those web components in a react app! React just wouldn't want to assume that blindly in their architecture, since, as you call out, a web component author would have to implement that, and only a subset of JS is serializable to JSON, and JS -> JSON can be information-lossy, and there are at least some non-trivial number of folks in the web components community that would see this as an anti-pattern (related to my callouts above). The tl;dr of how the React proposal for web component support was intended to work:

  1. prefer using web component instance properties when available
  2. if no property is available, use web component attributes, but only for a subset of "simple" value types

Your proposed solution/implementation works just fine with this "algorithm", but so do others, and React gets to avoid many (but not all) assumptions on implementation details as a result. Here's a toy example:

// ...
return <my-web-component myValue={JSON.stringify(myValue)} />;

In this case, "under the hood," react would first check if a property named myValue exists on the MyWebComponent instance. If it does not, it would set the myvalue attribute to the JSON string, because it's a string.

@hesxenon
Copy link

I think I did not get my point across. The links you provided actually strengthen my point that react should not omit attributes just because a corresponding property exists.

  • The first link (about the lists) states that we should use comma separated lists (not json encoded etc) for short multiples and elements (i.e. children) for lists of arbitrary length -> no complex value as attributes, we should express complex values through composition of declarative elements

  • the second link refers to how a JS API should expose objects and it even plainly states

    [WEBIDL] attributes should act like simple JavaScript object properties.

    Hinting towards the equivalency of properties and attributes with the latter being constrained by the declarative side of things

  • the third link is a number of "best practices" and while I disagree e.g. with the "always use shadow DOM" part the second point in "attributes and properties" clearly states to "always accept primitive data as either attributes or properties". This means I can't just build a proxy of some sort to hide the presence of a property from react just to keep the attribute present.

Let me finish with a simple task: please show me how to create a webcomponent that relies on the presence of an attribute (e.g. for styling purposes) AND has a property of the same name (following best practices) that isn't broken in react@19. E.g. recreate an input element with a readonly attribute/property pair.

Maybe this discussion is out of scope for this PR and I'd happily re-open this as an issue/discussion (if I can, that is) but as long as people have to adapt the way they write webcomponents to the way react is going to render them I think we can agree that that is at odds with the intent of webcomponents.

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Oct 17, 2024
@hesxenon
Copy link

bump

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Oct 17, 2024
@woody-li
Copy link

woody-li commented Nov 4, 2024

See also #19587
Web Components author may implement props via setAttribute with original variable type.

@TechQuery
Copy link

TechQuery commented Dec 7, 2024

I built a Web components wrapper of Apache ECharts: ECharts-JSX, which is based on Proxy object to transform uncountable official options to Custom Element properties, so React can't find a custom property defined on a DOM instance or its prototype, everything goes back to the old way setAttribute: https://idea2app.github.io/React-MobX-Bootstrap-ts/#/chart

But Preact, WebCell (my Web components framework) & some other view engines assign Object values to properties directly instead of setting attributes:

@luwes
Copy link

luwes commented Dec 8, 2024

I just went over @josepharhar doc and related threads and just wanted to echo what @cjpillsbury said. Not setting complex values like object/array/function when they are not detected (prop in element) was the plan. That would be a step in the right direction. Copied from the Google doc:

Objects are maybe properties (Preact without “[Object object]”)

Has the custom element upgraded yet? What is the type of the value? Will we use attribute or property?
No (or SSR) string/number attribute
No (or SSR) object/array/function Do nothing
Yes string/number If ‘propname’ in element,
Then use property
Else use attribute
Yes object/array/function If ‘propname’ in element,
Then use property
Else do nothing

Building further on this I do think it would be more useful for custom element implementers when the value is an object or array that it gets set as a property always (not requiring an in check). The doc mentions @matthewp's comment of the properties before upgrade problem but there is a solution for that which has been mentioned by @bahrus in this comment.

It's to make custom element properties lazy. The problem this solves is indeed very common and should be added to any well written custom element. Reproducing this problem is as easy as creating a custom element in JS and setting one of its props. Because it hasn't been added to the document or a manual customElements.upgrade(mediaController) hasn't been called the property is set on the prototype (not via the CE's defined setter).

const mediaController = document.createElement('media-controller');
mediaController.fullscreenElement = window;

Maybe I'm missing some information to why this wouldn't be a better solution, please let me know, but to me it looks like either the complex value (object/array/function) is lost OR it's set on the prototype of the unupgraded custom element instance and custom element authors have the option to revive that property value in their element when it is upgraded.

asafshen added a commit to descope/descope-js that referenced this issue Dec 22, 2024
## Related Issues

Fixes descope/etc#8299
## Description

The error occurs in React 19 due to changes in how React handles Custom
Elements and their properties.

before (18) React was more lenient when setting properties on Custom
Elements. If a property had only a getter and no setter, React would
often silently fail without throwing an error.
In React 19 - react has improved its support for Custom Elements and
aligns with the HTML specification. When React attempts to set a
property on a Custom Element, and the property has only a getter (no
setter), it throws a TypeError since it violates JavaScript’s property
descriptor rules

some sources:
https://blog.logrocket.com/working-custom-elements-react
facebook/react#29037

https://codingmall.com/knowledge-base/25-global/19398-how-does-react-19-support-custom-elements-differently-from-previous-versions

another approach will be also to change getters to internal
(`_getTheme`) and avoid setters, might work as well
Copy link

github-actions bot commented Mar 8, 2025

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Mar 8, 2025
@woody-li
Copy link

bump

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants