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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/renderers/dom/shared/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ var DOMPropertyOperations = {
* @param {string} name
* @param {*} value
*/
setValueForProperty: function(node, name, value) {
setValueForProperty: function(node, name, value, isCustomElement) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Expand Down Expand Up @@ -161,6 +161,8 @@ var DOMPropertyOperations = {
} else if (DOMProperty.isCustomAttribute(name)) {
DOMPropertyOperations.setValueForAttribute(node, name, value);
return;
} else if (isCustomElement) {
node[name] = value;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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__) {
Expand Down Expand Up @@ -218,7 +220,7 @@ var DOMPropertyOperations = {
* @param {DOMElement} node
* @param {string} name
*/
deleteValueForProperty: function(node, name) {
deleteValueForProperty: function(node, name, isCustomElement) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Expand All @@ -237,6 +239,8 @@ var DOMPropertyOperations = {
}
} else if (DOMProperty.isCustomAttribute(name)) {
node.removeAttribute(name);
} else if (isCustomElement) {
delete node[name];
}

if (__DEV__) {
Expand Down
76 changes: 29 additions & 47 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

custom elements now only receive element properties, not attributes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)} />).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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`', () => {
Expand All @@ -340,53 +359,16 @@ describe('ReactDOMComponent', () => {
expect(stubStyle.color).toEqual('green');
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


it('should reject attribute key injection attack on markup', () => {
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', () => {
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`', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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"');
}
Expand Down
12 changes: 5 additions & 7 deletions src/renderers/dom/stack/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] ||
Expand Down Expand Up @@ -1005,10 +1002,11 @@ ReactDOMComponent.Mixin = {
}
} else if (isCustomComponentTag) {
if (!RESERVED_PROPS.hasOwnProperty(propKey)) {
DOMPropertyOperations.setValueForAttribute(
DOMPropertyOperations.setValueForProperty(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (
Expand Down