React 15.2 Unkown Prop Warning

[jambonrose]

We're running react-datepicker v0.27 and have just upgraded to React 15.2. We are now raising the following error for each instance of the date-picker.

ERROR: 'Warning: Unknown props `date`, `locale`, `minDate`, `maxDate`, `excludeDates`, `includeDates`, `filterDate`, `dateFormat`, `onChangeDate` on <input> tag. Remove these props from the element. For details, see https://fb.me/react-unknown-prop

The link at the end of the output, for convenience: https://fb.me/react-unknown-prop

The discussion occurring at redux-form/redux-form#1249 may be of interest.

[martijnrusschen]

Interesting! I haven't used 15.2 myself yet, so can't confirm the issue. Seems we're hitting this error because of reason 2:

You are using a non-standard DOM attribute, perhaps to represent custom data. Custom data attributes should always have a data- prefix, as described in https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Using_data_attributes

@rafeememon Can you confirm this is the case. If so, we should start renaming our props to make sure these are all valid.

[sophiebits]

As the gist says, you want to change DateInput to "consume" the props it uses instead of blindly spreading all of them onto the input tag.

react-datepicker/src/date_input.jsx

Line 89 in 8da6d8a

{...this.props}

[gaearon]

This is problematic because you’re forwarding props that only make sense to your component (e.g. date) to the <input> component that doesn’t know what to do with them. In future versions of React we plan to pass such props “through” without the whitelist, so we ask that you don’t forward props that don’t make sense to the underlying components anymore.

Here is the code today:

    return <input
        ref='input'
        type='text'
        {...this.props}  <----------- can be anything! not good.
        value={this.state.value}
        onBlur={this.handleBlur}
        onChange={this.handleChange} />

you might have something like

    const {
      date, dateFormat , excludeDates, filterDate, includeDates, locale,
      maxDate, minDate, onBlur, onChange, onChangeDate,
      ...rest
    } = this.props;

    return <input
        ref='input'
        type='text'
        {...rest} <------------------ we know for sure we aren't passing any DateInput's props
        value={this.state.value}
        onBlur={this.handleBlur}
        onChange={this.handleChange} />

Alternatively you may encourage users to pass a prop called inputProps separately, and apply it instead:

    return <input
        ref='input'
        type='text'
        {...this.props.inputProps}
        value={this.state.value}
        onBlur={this.handleBlur}
        onChange={this.handleChange} />

Or you can maintain a custom whitelist if you’d like to:

    return <input
        ref='input'
        type='text'
        className={this.props.className} <-----
        style={this.props.style} <-------------  for example, could pass just these props
        value={this.state.value}
        onBlur={this.handleBlur}
        onChange={this.handleChange} />

[vladshcherbin]

I like the variant with ...rest, but sadly you'll get an eslint warning about variables, being not used.

[tricoder42]

@vladshcherbin If you aren't going to use all variables, you could use pick from lodash or similar function:

<input {...pick(this.props, ['value', 'onChange', ...])} />

[vladshcherbin]

@tricoder42 yes, but you are loosing an option to let user define some props later (className, onClick, etc). Probably _omit or similar will be better.

Anyway, there are a lot of ways to do this, I'm just not sure which to pick.

[tricoder42]

@vladshcherbin I guess you should define all props, that are valid for input
element. Might be useful to export them as constant InputProps and use them is such scenarios.

It would be definitely great, if all components could export their valid props, so we could use sth like: pick(this.props, Component.props)

[gaearon]

I like the variant with ...rest, but sadly you'll get an eslint warning about variables, being not used.

You can file this as an issue with ESLint. With the spread operator destructuring, I feel like this isn’t quite fair to call it unused.

Generally, if there is a pattern that works great to solve a pattern, you shouldn’t give up on it just because “ESLint says so”. ESLint is built by humans and can be wrong 😉 . And in any case you can override any rule in your config, or propose changes to popular configs.

That said, instead of

const { a, b, c, ...rest } = this.props

you can do

const rest = {...this.props} // or Object.assign({}, this.props) but don’t forget the polyfill
delete rest.a
delete rest.b
delete rest.c

It would be definitely great, if all components could export their valid props, so we could use sth like: pick(this.props, Component.props)

To me, this sounds a bit like a over-abstraction. I don’t think we’d like component authors to do something like this.

[benmvp]

We've run into this exact problem at Eventbrite.

We first started with the ...rest, but as @vladshcherbin pointed out it results in ESLint warnings because you have to define variables you ultimately aren't going to use in order for rest to contain the properties you expect.

Personally, I think this is a valid ESLint warning because you are declaring variables you aren't using. Our solution is just like @vladshcherbin's second suggestion to use _.omit. Those variables that would result in ESLint warnings because we aren't use them, we pass to _.omit to filter down rest even further:

let {usedA, usedB, usedC, ...extraAttrs} = this.props;
let propsToSpread = _.omit(extraAttrs, ['unusedD, unusedE']);

return (
    <input type="text" {...propsToSpread} />
);

Hopefully that gets the point across. It's similar to what @gaearon just suggested but maintains immutability (not calling delete) and is more functional.

[vladshcherbin]

There is already an Eslint issue, but it won't be addressed until Rest/Spread Properties would progress to higher stage.

[gaearon]

It's similar to what @gaearon just suggested but maintains immutability (not calling delete) and is more functional.

There is no problem with immutability in my suggestion. I suggested making a shallow copy first, so deleteing from that copy is fine.

[benmvp]

There is no problem with immutability in my suggestion. I suggested making a shallow copy first, so deleteing from that copy is fine.

My bad @gaearon. You're totally right. I saw the delete statements and couldn't see anything else hahaha