Skip to content

Instantly share code, notes, and snippets.

@statianzo
Last active August 30, 2016 13:47
Show Gist options
  • Star 5 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save statianzo/91234d26094ad89c1ff742f41b25d737 to your computer and use it in GitHub Desktop.
Save statianzo/91234d26094ad89c1ff742f41b25d737 to your computer and use it in GitHub Desktop.
unknown props warning
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);
@statianzo
Copy link
Author

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 and contactMethodId, that have already been used that I don't particularly care about. Also, redux connected components get the bonus dispatch 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.

@gaearon
Copy link

gaearon commented Jul 31, 2016

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>
);

@1000hz
Copy link

1000hz commented Jul 31, 2016

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!

@burakcan
Copy link

burakcan commented Jul 31, 2016

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

@gaearon
Copy link

gaearon commented Jul 31, 2016

@burakcan This is not a solution we recommend. Those modules are internal and may be changed/removed in any release.

@burakcan
Copy link

burakcan commented Aug 1, 2016

@gaearon, yes we're aware of that. But in that case, there's only one file to change.

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