-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
Attributes and properties for Custom Components #7249
Comments
I wanted to +1 this issue, I think it would really help out with Custom Element interop. I don't know if React needs to watch Any custom element built with Polymer (and I believe SkateJS) will have property setters auto-generated so you can rely on them being there for those elements. And we're encouraging developers to rely on properties as their source of truth when building custom elements. |
I'm leaning towards a properties-if-available model too. if (propName in obj) {
obj[propName] = value;
} else {
obj.setAttribute(propName, value);
} |
Properties-if-available is what we've done in Skate with our Incremental DOM wrapper FWIW. However, I've recently been messing around with an alternative approach, similar to the proposed That repo is only a playground for me at the moment, but I really like how you have full control over what is set as properties, attributes and events by having special |
Yup, we have first class props. |
There's a possibility of a raise condition between upgrading a custom element and running DOM reconciliation. For example:
Any thoughts? |
@blasten is correct. If an element is not-upgraded when React sets props, then attributes would be set, and once it's upgraded the element wouldn't have access to the props like it would with properties. |
+1 for what @blasten said. After having messed around with the method I linked to in my last comment, I can say it's a very robust approach. I'm not sure there is a fool proof method other than giving the consumer full control. A positive side effect is that there's less logic necessary to decide what to do. |
As a proof of concept, for this (and #7901) I've created a gist. It is a function that wraps any hyperscript style vdom function and enables full control over props, attributes and events (including custom events) for any vdom implementation that supports |
Thanks for putting that proposal together @treshugart. I think it would be great if all frameworks supported this contract:
React, Preact:render() {
return
<div ref="scroller" style={{ overflowY: 'auto' }}>
<custom-element props={{
onSomething: () => {},
ariaLabel: 'Tap',
scroller: () => this.ref.scroller
}}></custom-element>
</div>;
} class CustomElement extends HTMLElement {
set props(props) {
let prevProps = this._props;
// check prevProps.onSomething !== props.onSomething
// check prevProps.ariaLabel !== props.ariaLabel
this._props = props;
debounce(this._render);
}
get props() {
return Object.assign({}, this._props);
}
} The CE Declarative HTML<div id="scrollingElement" style="overflow-y: auto;">
<custom-element on-something="foo()" aria-label="tap" scroller="scrollingElement">
</custom-element>
</div> What I like about this setup is:
if (this.hasOwnProperty('props')) {
let userProps = self.props;
delete this.props;
this.props = userProps;
}
I'm curious about your guys thoughts. |
A possible proposal is for CE authors to always use the properties pattern that @blasten has shown, although I don't know if we want to encourage everyone to put everything into a single upgradeProperty(prop) {
if (this.hasOwnProperty(prop)) {
let value = this[prop];
delete this[prop];
this[prop] = value;
}
} Then the Custom Element author could use this approach with any properties their element exposes. Here's a jsbin that illustrates this technique. h/t to @sjmiles I believe if CE authors use this technique then React/Preact should be able to always set properties and not worry about attributes. And this will avoid situations where someone ends up with For events I still think it's preferable to use |
There are some cases, e. g. text input's |
I think the issue with setting both is that you can't assign an object or array to an attribute. You have to stringify it first. Setting both at the same time may create extra work on the element's behalf because depending on the order in which you set things, it'll likely need to process the change twice. I'm working with a teammate to produce some custom element examples that use the pattern I previously mentioned. I'll follow up on this thread when we publish what we've come up with to see what others think. |
@robdodson one property vs many properties is a personal choice, but one property (e.g.
|
yeah I agree there are advantages to just using First I wanted to see if we could encourage React and other libraries to always set properties, while at the same time encouraging Custom Element authors to adhere to the pattern of grabbing properties off of the instance if they've already been set. If both of those worlds start to gel, then the next step would be to explore ways to optimize it, possibly using the |
Objectively, discussing props as a single entry point for all properties is orthogonal to this issue, and highly contentious as a perf opt. Probably best to leave this to CE libs to implement if they see fit. Its prescriptive nature places it in a higher level of abstraction than the lower level browser APIs, and doesn't fall in line with how all elements expose their API as several top level props.
I still feel the consumer (React component authors) should have full control over what they want to do. If I want to set an attribute, or add an event listener, I should be able to do so without having to workaround React only setting props or an inadequate custom element implementation.
Everything always being properties is an ideal and we live in an imperfect world.
|
Yeah I'd be cool with this as well :) |
I agree that the scope of this thread isn't discussing such patterns. I just brought it up because I found it useful, but my intention wasn't to ask the react team to implement the CE integration that way. After all, they should support what is on the spec. I was just showing a potential pattern. I could have called my property "user" instead of "props" |
👍 that's an interesting pattern in itself, and definitely something that CE authors should consider :) |
Is there any investigation on this topic? It`s really annoying that we have still problems with custom components :/ |
We are not working on this but it makes sense to change the behavior to a more useful one in React 17. However it will require somebody to do the actual research into different options, weigh their pros and cons, and come up with a comprehensive proposal for the changes they want to see. Would you like to start working on this? |
I'm happy to help if I can. I'm not much of a React expert, but I think I have a pretty good idea of how things should work on the custom element side. I was wondering if there's been any thought toward supporting the same model that Preact uses? If it would help I could put together a doc that outlines their approach? |
I'm keen to see this feature implemented in React. Thanks a lot for your efforts making React great. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
Sorry, I have no other choice: +1 |
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 additional information, please include with in your comment! |
bump |
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! |
Still valid. Is there a way to disable the stale bot for this issue? It just spams & requires others to spam as well... |
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! |
I believe this should not be marked as stale. |
Bumping before it gets marked as stale again. |
Doing my part to foil the stale bot! IMHO, it's a crime that React is the only major JavaScript framework that does not support Web Components |
There's an active discussion in #11347 (comment) that you're welcome to contribute to if you care about this topic. |
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! |
Hey bot, bump to the bump, to the bump bump bump. Maybe this will be the last bump though? 🤞🏻 |
@edoardocavazza Not only
|
Do you want to request a feature or report a bug?
Feature
What is the current behavior?
Custom component's properties are always set as attribute.
What is the expected behavior?
Maybe React should watch at the static
observedAttributes
property for custom elements (https://w3c.github.io/webcomponents/spec/custom/) and then decide to set an attribute or an instance property. Otherwise, objects and array could be always passed as properties, in order to avoid<custom-element prop="[object Object]"></custom-element>
.The text was updated successfully, but these errors were encountered: