-
-
Save statianzo/91234d26094ad89c1ff742f41b25d737 to your computer and use it in GitHub Desktop.
const ContactBadge = (props) => ( | |
<div className='Badge' {...props}> | |
<div>{props.user.name}</div> | |
<div>{props.contactMethod.type}</div> | |
</div> | |
); | |
export default connect((state, {userId, contactMethodId}) => ({ | |
user: selectUser(state, userId), | |
contactMethod: selectContactMethod(state, contactMethodId) | |
}))(ContactBadge); |
//Using clone+delete to satisfy unknown props warning | |
const ContactBadge = (props) => { | |
const divProps = {...props}; | |
delete divProps.user; | |
delete divProps.contactMethod; | |
delete divProps.userId; | |
delete divProps.contactMethodId; | |
delete divProps.dispatch; | |
return ( | |
<div className='Badge' {...divProps}> | |
<div>{props.user.name}</div> | |
<div>{props.contactMethod.type}</div> | |
</div> | |
); | |
}; | |
export default connect((state, {userId, contactMethodId}) => ({ | |
user: selectUser(state, userId), | |
contactMethod: selectContactMethod(state, contactMethodId) | |
}))(ContactBadge); |
//Using destructuring to satisfy unknown props warning | |
const ContactBadge = ({user, contactMethod, userId, contactMethodId, dispatch, ...rest}) => ( | |
<div className='Badge' {...rest}> | |
<div>{user.name}</div> | |
<div>{contactMethod.type}</div> | |
</div> | |
); | |
export default connect((state, {userId, contactMethodId}) => ({ | |
user: selectUser(state, userId), | |
contactMethod: selectContactMethod(state, contactMethodId) | |
}))(ContactBadge); |
Here is the alternative API that in my opinion is cleaner and doesn’t have those drawbacks:
const ContactBadge = ({ user, contactMethod, divProps }) => (
<div className='Badge' {...divProps}>
<div>{user.name}</div>
<div>{contactMethod.type}</div>
</div>
);
In my opinion the original idea of passing all props is wrong by itself. This makes it harder for you to change component’s API (e.g. what if you want to add a prop which already has a meaning in HTML?) Separating your component’s props and the props you want to pass through makes more sense to me.
Even better would be to intentionally limit the props you pass, e.g. accept only style
and className
, or similar, because you’re sort of breaking encapsulation by enabling consumers to do everything with your div
.
const ContactBadge = ({ user, contactMethod, style, className }) => (
<div className='Badge' {...{style, className}}>
<div>{user.name}</div>
<div>{contactMethod.type}</div>
</div>
);
FWIW, I wrote a utility function filterOwnProps(reactElement)
that returns only props that don't belong to the current component. It's meant for class components, but you could probably adapt it to work with stateless functional components as well.
const filterOwnProps = (element) => {
const propTypes = element.constructor.propTypes
const props = {...element.props}
for (let key in propTypes) {
delete props[key]
}
return props
}
export default filterOwnProps
class MyComponent {
static propTypes = {
foo: React.PropTypes.string,
bar: React.PropTypes.string
}
render() {
<div {...filterOwnProps(this)}>
{foo}: {bar}
</div>
}
}
<MyComponent foo="foo" bar="bar" onClick={e => console.log(e)} /> // no Unknown props warning about foo and bar!
This is our solution in an app which is almost impossible to refactor every component:
// DOMElement.js
import React, { Component } from 'react';
import DOMProperty from 'react/lib/DOMProperty';
import EventPluginRegistry from 'react/lib/EventPluginRegistry';
import pick from 'lodash/pick';
import keys from 'lodash/keys';
import concat from 'lodash/concat';
const reactKeys = [
'children',
'dangerouslySetInnerHTML',
'autoFocus',
'defaultValue',
'valueLink',
'defaultChecked',
'checkedLink',
'innerHTML',
'suppressContentEditableWarning',
'onFocusIn',
'onFocusOut',
'onFocus',
];
const DOMKeys = keys(DOMProperty.properties);
const eventKeys = keys(EventPluginRegistry.registrationNameModules);
const pickProps = pick(
concat(
reactKeys,
concat(DOMKeys, eventKeys)
)
);
export default function domElement(Komponent) {
/* eslint-disable react/prefer-stateless-function */
return class DOMElement extends Component {
render() {
return <Komponent { ...pickProps(this.props) } />;
}
};
}
// Div.js
import domElement from './DOMElement';
export default domElement('div');
// Usage:
import Div from 'Div'
...
<Div foo="bar" baz /> // No errors
@burakcan This is not a solution we recommend. Those modules are internal and may be changed/removed in any release.
@gaearon, yes we're aware of that. But in that case, there's only one file to change.
My pain with the unknown props warning is the amount of boilerplate that has to get added, especially when wrapping with something like react-redux. In this instance the
ContactBadge
component passes props through to its root<div>
so that consumers can add things like click event handlers.current.js
is the way I could write pre-15.2.In
delete.js
, the render method is dominated by prop sanitizing rather than being focused on what really gets rendered. A side note is that I miss out on the terseness of an implicit return.In
destructure.js
I'm forced to destructure out props,userId
andcontactMethodId
, that have already been used that I don't particularly care about. Also, redux connected components get the bonusdispatch
prop to strip out as well. ESlint complains about unused variables as well. It's obviously possible to change my eslint config, but that's a rule I'd prefer to keep enabled.In general, I've appreciated the warnings React provides. However, in this case I feel like the added noise to the code defeats the benefit.