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

Passing props to custom elements as properties instead of attributes #8755

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@joeldenning
Contributor

joeldenning commented Jan 11, 2017

This pull request introduces a breaking change to how React handles custom elements / web components. The change is that React props are currently passed to custom elements via setAttribute, but now are passed as properties on the dom element itself.

See discussion in #7901, specifically this comment where @sebmarkbage indicated that this change would be part of the next major release of React. Since react@16.0.0-alpha was released yesterday and is on the master branch, I'm hoping that now is the right time for this breaking change to make it into the master branch. I'm not sure if anyone else was planning on implementing this or not.

Notes:

  • This makes it a lot easier to pass non-string data down to custom elements (e.g., objects can be passed without getting stringified to [Object object]). Instead of <custom-element ref={el => el.myProp = myProp} />, you can just do <custom-element myProp={myProp} />.
  • This does not solve the problem of how to interoperate with events fired by custom elements. If this pr is merged, I will follow it up with another one to tackle that problem, using the proposed solution in #7901 as a guide.
  • I am not doing Object.assign(element, props) (like @sebmarkbage suggested) because that does not delete any props that are passed conditionally to the custom element. Also, Object.assign(element, props) bypasses the events recorded in ReactHostOperationHistory, which seems like another reason against doing so. Not sure if ReactHostOperationHistory is important or not for custom elements, though.
  • I did not change the corresponding ReactDOMFiberComponent because I wasn't sure if I should do so.
  • The linter was broken for me on the master branch before I made any changes, but my changes did not introduce any new warnings or errors as far as I can tell.
@@ -130,7 +130,7 @@ var DOMPropertyOperations = {
* @param {string} name
* @param {*} value
*/
setValueForProperty: function(node, name, value) {
setValueForProperty: function(node, name, value, isCustomElement) {

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Not sure if there is a better way to do this. I want custom properties on custom elements to be set, but the next few lines of code prevent that from happening because DOMProperties.properties cannot possible have a propertyInfo for a custom property name.

The reason why I'm calling this function at all is because I want the instrumentation code in this function to run. Otherwise I would consider just doing node[propName] = propValue inside of ReactDOMComponent itself.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Not sure if there is a better way to do this. I want custom properties on custom elements to be set, but the next few lines of code prevent that from happening because DOMProperties.properties cannot possible have a propertyInfo for a custom property name.

The reason why I'm calling this function at all is because I want the instrumentation code in this function to run. Otherwise I would consider just doing node[propName] = propValue inside of ReactDOMComponent itself.

@@ -161,6 +161,8 @@ var DOMPropertyOperations = {
} else if (DOMProperty.isCustomAttribute(name)) {
DOMPropertyOperations.setValueForAttribute(node, name, value);
return;
} else if (isCustomElement) {
node[name] = value;

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Like I described above, custom properties on custom elements won't have a propertyInfo definition, but we still want to set them on the element.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Like I described above, custom properties on custom elements won't have a propertyInfo definition, but we still want to set them on the element.

@@ -218,7 +220,7 @@ var DOMPropertyOperations = {
* @param {DOMElement} node
* @param {string} name
*/
deleteValueForProperty: function(node, name) {
deleteValueForProperty: function(node, name, isCustomElement) {

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

See my comments above for setValueForProperty -- I'm changing this function for much the same reason.

@joeldenning

joeldenning Jan 11, 2017

Contributor

See my comments above for setValueForProperty -- I'm changing this function for much the same reason.

@@ -318,13 +317,33 @@ describe('ReactDOMComponent', () => {
expect(container.firstChild.className).toEqual('');
});
it('should properly update custom attributes on custom elements', () => {
it('should properly update custom properties on custom elements', () => {

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

custom elements now only receive element properties, not attributes.

@joeldenning

joeldenning Jan 11, 2017

Contributor

custom elements now only receive element properties, not attributes.

This comment has been minimized.

@justinfagnani

justinfagnani Jan 12, 2017

How would one set an attribute on a custom element then?

@justinfagnani

justinfagnani Jan 12, 2017

How would one set an attribute on a custom element then?

This comment has been minimized.

@joeldenning

joeldenning Jan 12, 2017

Contributor

If this pull request is merged, then the only way to set attributes on a custom element would be with refs (<custom-element ref={el => el.setAttribute('my-attr', val)} />).

@joeldenning

joeldenning Jan 12, 2017

Contributor

If this pull request is merged, then the only way to set attributes on a custom element would be with refs (<custom-element ref={el => el.setAttribute('my-attr', val)} />).

This comment has been minimized.

@joeldenning

joeldenning Jan 12, 2017

Contributor

Note that when you change many native dom properties (such as node.id and node.className) that the corresponding attributes (id and class) are automatically changed to reflect the property value. So users will still be able to effectively set the id and class attributes (among others), by changing the id and className properties on the element.

@joeldenning

joeldenning Jan 12, 2017

Contributor

Note that when you change many native dom properties (such as node.id and node.className) that the corresponding attributes (id and class) are automatically changed to reflect the property value. So users will still be able to effectively set the id and class attributes (among others), by changing the id and className properties on the element.

@@ -340,53 +359,16 @@ describe('ReactDOMComponent', () => {
expect(stubStyle.color).toEqual('green');
});
it('should reject attribute key injection attack on markup', () => {

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Tbh I don't really know what "attribute key injection attack on markup" is. This test started failing when I changed the code because it's expecting attributes to be set instead of properties. However, my guess is that this attack vector doesn't apply anymore now that we're not actually putting the react props into the markup as attributes.

Not 100% sure, though, would love to hear others' thoughts on it.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Tbh I don't really know what "attribute key injection attack on markup" is. This test started failing when I changed the code because it's expecting attributes to be set instead of properties. However, my guess is that this attack vector doesn't apply anymore now that we're not actually putting the react props into the markup as attributes.

Not 100% sure, though, would love to hear others' thoughts on it.

This comment has been minimized.

@gaearon

gaearon Jan 12, 2017

Member

Maybe it is specific to server rendering? I haven't actually checked.

@gaearon

gaearon Jan 12, 2017

Member

Maybe it is specific to server rendering? I haven't actually checked.

);
});
it('should reject attribute key injection attack on update', () => {

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Same here about this probably no longer being applicable now that things are passed as properties instead of attributes.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Same here about this probably no longer being applicable now that things are passed as properties instead of attributes.

@@ -566,7 +566,7 @@ ReactDOMComponent.Mixin = {
div.innerHTML = `<${type}></${type}>`;
el = div.removeChild(div.firstChild);
} else if (props.is) {
el = ownerDocument.createElement(type, props.is);
el = ownerDocument.createElement(type, {is: props.is});

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

See custom elements spec for why I changed this. It explains that you create customized built-in elements like so:

/* Method 1: the "is" attribute in the markup
 * React doesn't do this because it calls createElement instead of setting innerHTML
 */
document.body.innerHTML = '<button is="my-button"></button>';

/* Method 2: the "is" option when calling createElement
 * React _needs_ to do this in order for the browser to correctly upgrade an
 * element to be a customized built-in element.
 */
document.createElement('button', {is: 'my-button'});
@joeldenning

joeldenning Jan 11, 2017

Contributor

See custom elements spec for why I changed this. It explains that you create customized built-in elements like so:

/* Method 1: the "is" attribute in the markup
 * React doesn't do this because it calls createElement instead of setting innerHTML
 */
document.body.innerHTML = '<button is="my-button"></button>';

/* Method 2: the "is" option when calling createElement
 * React _needs_ to do this in order for the browser to correctly upgrade an
 * element to be a customized built-in element.
 */
document.createElement('button', {is: 'my-button'});
getNode(this),
propKey
);
DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey, true);

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

React props are now dom element properties, not attributes. The true is to indicate that this is a custom element node.

@joeldenning

joeldenning Jan 11, 2017

Contributor

React props are now dom element properties, not attributes. The true is to indicate that this is a custom element node.

@@ -1005,10 +1002,11 @@ ReactDOMComponent.Mixin = {
}
} else if (isCustomComponentTag) {
if (!RESERVED_PROPS.hasOwnProperty(propKey)) {
DOMPropertyOperations.setValueForAttribute(
DOMPropertyOperations.setValueForProperty(

This comment has been minimized.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Same here - React props are now dom element properties, not attributes. The true once again indicates that this is a custom element node.

@joeldenning

joeldenning Jan 11, 2017

Contributor

Same here - React props are now dom element properties, not attributes. The true once again indicates that this is a custom element node.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 12, 2017

Member

It seems like the consensus seems to be moving towards that setting only properties seem to be too restrictive.

I think what we'll do instead is check for the existence of a property and then use it, if there is no property already we will use the attribute instead.

However, even then it is unclear what to do about events. Probably a convention based approach.

It's also unclear when we should check for the existence of a property. Could we check that early, e.g. on the prototype, or do we have to do it on the fly on the instance every time?

So I'm not sure we're ready to make this move yet without further consideration.

Member

sebmarkbage commented Jan 12, 2017

It seems like the consensus seems to be moving towards that setting only properties seem to be too restrictive.

I think what we'll do instead is check for the existence of a property and then use it, if there is no property already we will use the attribute instead.

However, even then it is unclear what to do about events. Probably a convention based approach.

It's also unclear when we should check for the existence of a property. Could we check that early, e.g. on the prototype, or do we have to do it on the fly on the instance every time?

So I'm not sure we're ready to make this move yet without further consideration.

@justinfagnani

This comment has been minimized.

Show comment
Hide comment
@justinfagnani

justinfagnani Jan 12, 2017

@sebmarkbage in #7249 we've pointed out that property-if-exists breaks under upgrades.

justinfagnani commented Jan 12, 2017

@sebmarkbage in #7249 we've pointed out that property-if-exists breaks under upgrades.

@joeldenning

This comment has been minimized.

Show comment
Hide comment
@joeldenning

joeldenning Jan 12, 2017

Contributor

@justinfagnani That's an interesting point. Does it only break if the custom element does not respect the convention that "attributes are initial configuration, but properties are the source of truth"? Is that something we could live with?

If not, then yeah maybe we have to provide explicit ways to provide both attributes and properties, even though that is ugly:

<custom-element properties={{prop1: 'val1', prop2: 'val2'}} attributes={{attr1: 'str1', attr2: 'str2'}} />

Or:

<custom-element properties={{prop1: 'val1', prop2: 'val2'}} attr1="str1" />

I find both of the above examples sort of awkward, but maybe that is what has to happen? I'm hoping we could avoid doing something like that.

Contributor

joeldenning commented Jan 12, 2017

@justinfagnani That's an interesting point. Does it only break if the custom element does not respect the convention that "attributes are initial configuration, but properties are the source of truth"? Is that something we could live with?

If not, then yeah maybe we have to provide explicit ways to provide both attributes and properties, even though that is ugly:

<custom-element properties={{prop1: 'val1', prop2: 'val2'}} attributes={{attr1: 'str1', attr2: 'str2'}} />

Or:

<custom-element properties={{prop1: 'val1', prop2: 'val2'}} attr1="str1" />

I find both of the above examples sort of awkward, but maybe that is what has to happen? I'm hoping we could avoid doing something like that.

@treshugart

This comment has been minimized.

Show comment
Hide comment
@treshugart

treshugart May 9, 2017

@joeldenning, @justinfagnani

tl;dr setting props all of the time while giving the consumer an escape hatch would be my preference.

Longer version:

Does it only break if the custom element does not respect the convention that "attributes are initial configuration, but properties are the source of truth"? Is that something we could live with?

I'm not sure that requiring that attributes be initial configuration would be the best assumption to make. In the event that you require a complex data type, but the element hasn't been upgraded, this requires that the author provide a way to deserialise the information from the attributes and also makes the assumption that the data type provided can be serialised.

I find both of the above examples sort of awkward, but maybe that is what has to happen? I'm hoping we could avoid doing something like that.

It is. In practice, however, I've found that you can get away with using props 99% of the time. The time you need to break away from this is when setting aria-* or data-* attributes. If you handle these, then you cover most use cases.

// result (unless CE author reflects prop to attr): <custom-element />
<custom-element someProp={{ foo: 'bar' }} />

It's up to the CE author to reflect the prop to the attribute (id does this for example).

In https://github.com/skatejs/val, we've made aria and data special props:

// result: <custom-element aria-describedby="something" data-foo="bar" />
<custom-element aria={{ describedby: 'something' }} data={{ foo: 'bar' }} />

I assume React may want to make this more like the following, though:

// result: <custom-element aria-describedby="something"  data-foo="bar" />
<custom-element someProp={{ foo: 'bar' }} aria-describedby="something" data-foo="bar" />

Other edge cases may be if you require attributes for styling and reflection is not guaranteed for the prop (value is one that comes to mind, if the user has entered a value), or if a custom element (or otherwise) is designed to favour attributes - which we should definitely discourage as a community effort.

I think if props can always be set, but give an escape hatch for attributes - as well as aria and data - then React would have a fantastic balance of ergonomics and consumer control.

treshugart commented May 9, 2017

@joeldenning, @justinfagnani

tl;dr setting props all of the time while giving the consumer an escape hatch would be my preference.

Longer version:

Does it only break if the custom element does not respect the convention that "attributes are initial configuration, but properties are the source of truth"? Is that something we could live with?

I'm not sure that requiring that attributes be initial configuration would be the best assumption to make. In the event that you require a complex data type, but the element hasn't been upgraded, this requires that the author provide a way to deserialise the information from the attributes and also makes the assumption that the data type provided can be serialised.

I find both of the above examples sort of awkward, but maybe that is what has to happen? I'm hoping we could avoid doing something like that.

It is. In practice, however, I've found that you can get away with using props 99% of the time. The time you need to break away from this is when setting aria-* or data-* attributes. If you handle these, then you cover most use cases.

// result (unless CE author reflects prop to attr): <custom-element />
<custom-element someProp={{ foo: 'bar' }} />

It's up to the CE author to reflect the prop to the attribute (id does this for example).

In https://github.com/skatejs/val, we've made aria and data special props:

// result: <custom-element aria-describedby="something" data-foo="bar" />
<custom-element aria={{ describedby: 'something' }} data={{ foo: 'bar' }} />

I assume React may want to make this more like the following, though:

// result: <custom-element aria-describedby="something"  data-foo="bar" />
<custom-element someProp={{ foo: 'bar' }} aria-describedby="something" data-foo="bar" />

Other edge cases may be if you require attributes for styling and reflection is not guaranteed for the prop (value is one that comes to mind, if the user has entered a value), or if a custom element (or otherwise) is designed to favour attributes - which we should definitely discourage as a community effort.

I think if props can always be set, but give an escape hatch for attributes - as well as aria and data - then React would have a fantastic balance of ergonomics and consumer control.

@trusktr

This comment has been minimized.

Show comment
Hide comment
@trusktr

trusktr Jul 28, 2017

To me,

<custom-element properties={{prop1: 'val1', prop2: 'val2'}} attr1="str1" />

is the best option, as it conflicts the least with standard HTML behavior.

I would even say that

<custom-element _properties={{prop1: 'val1', prop2: 'val2'}} attr1="str1" />

is even better because less possible collision, and then the React API user has explicit control without React having to guess. If React guesses things, there's going to be conflicts.

Even

<custom-element _properties={{prop1: 'val1', prop2: 'val2'}} attr1="str1" />

can conflict, because having a _properties attribute is completely legal in vanilla HTML, and someone may eventually have an issue with it, so even that needs a react-specific escape hatch.

I personally would rather have #10070 because then my custom elements would be more likely to have performance improvements across many more frameworks than just React, because setAttribute is standard.

trusktr commented Jul 28, 2017

To me,

<custom-element properties={{prop1: 'val1', prop2: 'val2'}} attr1="str1" />

is the best option, as it conflicts the least with standard HTML behavior.

I would even say that

<custom-element _properties={{prop1: 'val1', prop2: 'val2'}} attr1="str1" />

is even better because less possible collision, and then the React API user has explicit control without React having to guess. If React guesses things, there's going to be conflicts.

Even

<custom-element _properties={{prop1: 'val1', prop2: 'val2'}} attr1="str1" />

can conflict, because having a _properties attribute is completely legal in vanilla HTML, and someone may eventually have an issue with it, so even that needs a react-specific escape hatch.

I personally would rather have #10070 because then my custom elements would be more likely to have performance improvements across many more frameworks than just React, because setAttribute is standard.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 5, 2018

Member

I'll close this as stale and lacking consensus. The best way to build consensus is to submit an RFC. Our latest thinking on this problem is in #11347 (comment).

Member

gaearon commented Jan 5, 2018

I'll close this as stale and lacking consensus. The best way to build consensus is to submit an RFC. Our latest thinking on this problem is in #11347 (comment).

@gaearon gaearon closed this Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment