React: Unknown prop warning, ignore undefined?

[syranide]

Warning: Unknown prop ... on ... tag. Remove this prop from the element.

These are emitted even if the property value is undefined. Should undefined perhaps be treated as not existing perhaps? That's how it is intended AFAIK.

[jimfb]

Someone mentioned this a while back. Dan and I discussed it, and came to the conclusion that the current behavior is good, because the only case we could come up with for having the prop be undefined is if the prop was specified on the element and now you are trying to change it to be undefined. The prop should never have been specified on that element in the first place.

If you're interested: https://gist.github.com/jimfb/d99e0678e9da715ccf6454961ef04d1b#gistcomment-1819019

The TLDR is:

However this sounds like an anti-pattern. If you’re trying to remove a prop post factum, it probably means it shouldn’t have been there in the first place.

If there is a use case we're missing, let us know!

[jimfb]

Best practice would be:

{myCustomProp1, myCustomProp2, ...otherProps} =  this.props;
return <div ...otherProps />;

Alternate practice would be:

var otherProps = Object.assign({}, this.props);
delete otherProps.myCustomProp1;
delete otherProps.myCustomProp2;
return <div ...otherProps />;

Bad practice would be to assign the props to be undefined:

var element = <div ...this.props />;
element = React.cloneElement(element, {myCustomProp1: undefined});
element = React.cloneElement(element, {myCustomProp2: undefined});
return element;

[syranide]

var otherProps = Object.assign({}, this.props);
delete otherProps.myCustomProp1;
delete otherProps.myCustomProp2;
return <div ...otherProps />;

@jimfb That's exactly my use-case, and my reasoning was that delete can be quite expensive and it's faster to simply assign undefined. I doubt it can be measured in practice, so I don't really care. Just thought it was worth bringing up (as it's generally a good idea), but for React I agree there's probably no real reason to do this. :+1:

[oztune]

Along these same lines, is there a way to achieve the same prop deleting behavior with React.cloneElement?

[jimfb]

@oztune Sort of, but it is probably a very bad practice. In general, the illegal prop should have never been attached to that DOM node in the first place. That's the thing to fix.

However, If you really need to do this, rather than using cloneElement, you would manually pull the type, key, props, and ref off the old element and call React.createElement with your new props.

[jquense]

hehe, just bit by this. The logic behind the choice makes sense, however it's not intuitive given that the more widely used heuristic for "provided" prop is === undefined (or == null even) throughout the rest of React. TBF tho @syranide's point is valid, delete is slower, tho probably not prohibitively, but who could even measure such a thing.

[taion]

I agree with @syranide's issue. We have this use case in the context of:

class NavItem extends React.Component {
  onClick = () => {
    const { onSelect, eventKey } = this.props;
    onSelect(eventKey);
  };

  render() {
    const props = { ...this.props };
    delete props.onSelect;
    delete props.eventKey;

    return (
      <div {...props} onClick={this.onClick} />
    );
  }
}

Doing e.g.

const { onSelect, eventKey, ...props } = this.props;

is not ideal here because it doesn't in general pass "unused variables" linting rules.

My preference here would be to do:

const props = { ...this.props };
props.onSelect = undefined;
props.eventKey = undefined;

exactly to avoid the deoptimization of the object away from fast mode here. I want to avoid using e.g. Lodash omit specifically because that one makes a bunch of extra passes through the object and is probably worse than just using delete.

[syranide]

@taion Not agreeing/disagreeing. I think the only real argument for keeping it as-is is that I think you'll find a lot of people taking the easy route and "unintentionally" passing around undefined (rather than null), so you'd have cases where you've specified an invalid prop but the value is undefined and thus not showing a warning (but then again you only need to see the warning once). Is this really a problem in practice? Rarely I'd guess.

const { onSelect, eventKey, ...props } = this.props;
onSelect; eventKey; // unused

Not saying it doesn't suck, but that works and that's how you would otherwise do it (although rare in JS).

[taion]

@syranide

That actually violates the no-unused-expressions Lint rule (enabled in Airbnb's config, which we use – though not in eslint:recommended or in eslint-config-fbjs... though comment there suggests it should be enabled with allowShortCircuit set to true)

The delete isn't the end of the world; just saying I'd rather set to undefined, but not by much.

[jquense]

I like @itajaja s approach of using ignore pattern in eslint { foo: _, ...props } :P