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
Jump to file or symbol
Failed to load files and symbols.
+40 −56
Diff settings

Always

Just for now

Passing props to custom elements as properties instead of attributes.

  • Loading branch information...
joeldenning committed Jan 11, 2017
commit 52e613531de83f03ca721807b4582dd01df036e5
@@ -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.

var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
if (propertyInfo) {
@@ -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.

}
if (__DEV__) {
@@ -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.

var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
if (propertyInfo) {
@@ -237,6 +239,8 @@ var DOMPropertyOperations = {
}
} else if (DOMProperty.isCustomAttribute(name)) {
node.removeAttribute(name);
} else if (isCustomElement) {
delete node[name];
}
if (__DEV__) {
@@ -249,8 +249,7 @@ describe('ReactDOMComponent', () => {
expect(lightDOM[0].getAttribute('slot')).toBe('first');
expect(lightDOM[1].getAttribute('slot')).toBe('second');
} );
});
it('should skip reserved props on web components', () => {
var container = document.createElement('div');
@@ -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.

var container = document.createElement('div');
ReactDOM.render(<some-custom-element foo="bar"/>, container);
ReactDOM.render(<some-custom-element bar="buzz"/>, container);
var node = container.firstChild;
expect(node.hasOwnProperty('foo')).toBe(false);
expect(node.bar).toBe('buzz');
});
it('should not set attributes for props passed to custom elements', () => {
var container = document.createElement('div');
ReactDOM.render(<some-custom-element foo="bar"/>, container);
var node = container.firstChild;
expect(node.hasAttribute('foo')).toBe(false);
});
it('should pass complex data to custom elements as properties on the dom node', () => {
var container = document.createElement('div');
var anArray = ['1', 2, new Date(), {}, [], false];
var date = new Date();
ReactDOM.render(<some-custom-element foo={{bar: 'baz'}} anArray={anArray} aNumber={2} aDate={date}/>, container);
var node = container.firstChild;
expect(node.hasAttribute('foo')).toBe(false);
expect(node.getAttribute('bar')).toBe('buzz');
expect(node.foo).toEqual({bar: 'baz'});
expect(node.anArray).toEqual(anArray);
expect(node.aNumber).toEqual(2);
expect(node.aDate).toEqual(date);
});
it('should clear a single style prop when changing `style`', () => {
@@ -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.

spyOn(console, 'error');
for (var i = 0; i < 3; i++) {
var container = document.createElement('div');
var element = React.createElement(
'x-foo-component',
{'blah" onclick="beevil" noise="hi': 'selected'},
null
);
ReactDOM.render(element, container);
}
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toEqual(
'Warning: Invalid attribute name: `blah" onclick="beevil" noise="hi`'
);
});
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.

spyOn(console, 'error');
for (var i = 0; i < 3; i++) {
var container = document.createElement('div');
var beforeUpdate = React.createElement('x-foo-component', {}, null);
ReactDOM.render(beforeUpdate, container);
var afterUpdate = React.createElement(
'x-foo-component',
{'blah" onclick="beevil" noise="hi': 'selected'},
null
);
ReactDOM.render(afterUpdate, container);
}
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toEqual(
'Warning: Invalid attribute name: `blah" onclick="beevil" noise="hi`'
);
});
it('should update arbitrary attributes for tags containing dashes', () => {
it('should update arbitrary properties for tags containing dashes', () => {
var container = document.createElement('div');
var beforeUpdate = React.createElement('x-foo-component', {}, null);
ReactDOM.render(beforeUpdate, container);
var afterUpdate = <x-foo-component myattr="myval" />;
var afterUpdate = <x-foo-component myProperty="myval" />;
ReactDOM.render(afterUpdate, container);
expect(container.childNodes[0].getAttribute('myattr')).toBe('myval');
expect(container.childNodes[0].myProperty).toBe('myval');
});
it('should clear all the styles when removing `style`', () => {
@@ -595,10 +577,10 @@ describe('ReactDOMComponent', () => {
expect(nodeValueSetter.mock.calls.length).toBe(3);
});
it('should ignore attribute whitelist for elements with the "is: attribute', () => {
it('should ignore property whitelist for elements with the "is" attribute', () => {
var container = document.createElement('div');
ReactDOM.render(<button is="test" cowabunga="chevynova"/>, container);
expect(container.firstChild.hasAttribute('cowabunga')).toBe(true);
expect(container.firstChild.hasOwnProperty('cowabunga')).toBe(true);
});
it('should not update when switching between null/undefined', () => {
@@ -1057,7 +1039,7 @@ describe('ReactDOMComponent', () => {
var container = document.createElement('div');
spyOn(document, 'createElement').and.callThrough();
ReactDOM.render(<div is="custom-div" />, container);
expect(document.createElement).toHaveBeenCalledWith('div', 'custom-div');
expect(document.createElement).toHaveBeenCalledWith('div', {is: 'custom-div'});
} else {
expect(ReactDOMServer.renderToString(<div is="custom-div" />)).toContain('is="custom-div"');
}
@@ -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'});
} else {
// Separate else branch instead of using `props.is || undefined` above becuase of a Firefox bug.
// See discussion in https://github.com/facebook/react/pull/6896
@@ -953,10 +953,7 @@ ReactDOMComponent.Mixin = {
// Do nothing for event names.
} else if (isCustomComponent(this._tag, lastProps)) {
if (!RESERVED_PROPS.hasOwnProperty(propKey)) {
DOMPropertyOperations.deleteValueForAttribute(
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.

}
} else if (
DOMProperty.properties[propKey] ||
@@ -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.

getNode(this),
propKey,
nextProp
nextProp,
true
);
}
} else if (
ProTip! Use n and p to navigate between commits in a pull request.