Warn when propTypes does not specify a received prop

[erikbrannstrom]

When developing, it's easy to occasionally forget about specifying all component props in propTypes. Once you've gone on for a while without doing this, identifying and adding all these missing props is quite the hassle.

This pull request adds a warning when a prop is not specified in propTypes, however only if propTypes is actually defined (an empty object is fine). This is pretty much was discussed in issue #1587.

I willingly admit I am slightly out of my depth here, especially regarding the code in ReactDescriptorValidator.js. The code is duplicated between that file and ReactCompositeComponent.js, but it seems to be the case with the existing validation as well.

I've added a test case for this and it succeeded, though since 04c982 it seems I can't run any tests at all (even on master).

It might be this feature should only be enabled after the user explicitly says so, but I thought I'd send this in anyway for discussion.

[syranide]

I've added a test case for this and it succeeded, though since 04c982 it seems I can't run any tests at all (even on master).

Try rm node_modules -r and npm install, that usually does it for me when tests no longer work.

[sophiebits]

It sounds like there may have been a bad merge there:

#1601 (comment)

[erikbrannstrom]

Tests are back online, yay! Of course I had messed up the test case, but it's fixed now (added empty propTypes on component, since _checkPropTypes won't run if propTypes is undefined).

I realize now that it probably would have been better if I'd merged rather than rebased all other commits. Sorry about that...

[syranide]

@erikbrannstrom Rebasing is fine, but you need to rebase your local master branch first (i.e., pull against upstream) and then rebase your PR-branch against your local master branch.

[erikbrannstrom]

@syranide Thanks, good to know! Should I just leave this as is?

[syranide]

@erikbrannstrom I think you need to fix it. Don't quote me on this (so copy relevant files first if necessary), but I think if you pull/rebase master against upstream master, and then rebase this against your local master (and force push) that this should be corrected.

[erikbrannstrom]

@syranide That seems to have done the trick, sorry about the screw up!

[jordwalke]

I'm glad we only do this in __DEV__!

[syranide]

@erikbrannstrom No problem! It wasn't long ago that I was just as git-confused myself (ahem #459 :)).

[zpao]

This is fine for simple components, but a very common pattern is for a component to wrap and extend an existing component.

Pulling from your test, let's pretend FancyComponent exists. It's sole purpose is to be a specialty version of Component. Ignore the fact the this composition here doesn't really make any sense.

    var Component = React.createClass({
      propTypes: {
        type: React.PropTypes.string
      },
      render: function() {
        return <span>{this.props.prop}</span>;
      }
    });

    var FancyComponent = React.createClass({
      propTypes: {
        color: React.PropTypes.string
      },
      render: function() {
        var {color, ...props} = this.props;
        // do something with color maybe
        return <Component {...props} />;
      }
    });

    ReactTestUtils.renderIntoDocument(<FancyComponent color="green" type="alert" />);

This is going to warn me - 'Warning: Prop 'type' was not expected in 'FancyComponent'.', but it's actually a totally valid and encouraged use. I mentioned this in #1587 (comment)

We've talked about doing this for a while and I think we'll get there, but we need to build this composition knowledge in better. Until we have a clearly defined path for that, this is just going to be really noisy.

So put simply, while this is a great effort, we just can't take it :/

[erikbrannstrom]

No problem! I wrote this to identify missing prop specs in the project I'm currently working on, and thought I might as well send a pull request to get a discussion going.

Noise would indeed be a problem. My initial thought was writing it as an addon with start and stop functions (Perf-style!), but I didn't find an obvious way to hook in to the prop validations.

Oh well, this should probably be kept to the related issue rather than this pull request. Keep up the good work!

[mkreidenweis]

@zpao Preventing the issue that you mentioned above is easy now using the new ES6 syntax with object spread operator, right?.
Example with both functional and class syntax:

const Component = (props) => {
  return <span>{props.type}</span>;
};
Component.propTypes = {
  type: React.PropTypes.string
};

class FancyComponent extends React.Component {
  render() {
    console.log(Component.propTypes);
    var {color, ...props} = this.props;
    // do something with color maybe
    return <Component {...props} />;
  }
}
FancyComponent.propTypes = {
  ...Component.propTypes, // ⇐ ■ look here ■
  color: React.PropTypes.string
};

I consider it a lot cleaner explicitly specifying that the FancyComponent accepts all the Component's props as well, compared to just accepting them without declaration.

Giving circumstances changed now, would you reconsider accepting a change like the one proposed in this PR?