Validating PropTypes when a class is created

[rppc]

Note: This is the same pull request. I've cleaned up the commit history.

Following this bug report, the PropTypes are now checked in development mode when the class is created, so that, for instance, the call:

var MyComponent = React.createClass({
        displayName: 'Component',
        propTypes: {
            optionalNumber: React.PropTypes.number,
            optionalEnum: React.PropTypes.oneOf('foo'),       // Invalid.
          },
          render: function() {
            return <span>{this.props.prop}</span>;
          },
        });

will result in a warning being logged, and optionalEnum being cleared from the propTypes object.

Up to this point, this check only occurred when ReactElement.createElement was called in development mode. Some refactoring might be needed in order to avoid unnecessary tests when ReactElement.createElement is called in development mode.

Test cases were included in the file ReactClass-test.js (see the last test).

[phss]

This is a bit of a newbie question but why not validate for ContextType as well? As far as I understand, they are defined in the same way as PropTypes. Shouldn't they also raise a warning if defined in an invalid way?

[rppc]

You're probably right. My knowledge of React is very limited, as this is my first contribution ever.

[zpao]

@phss That's a point I was going to bring up while reviewing :)

I appreciate you taking the time to do this and I know it was a pain to work through git issues but I have a fundamental change to make.

We should simplify this and just put some if (__DEV__) { if (invalidPropType) { warning(…) } } blocks inside ReactPropTypes. ReactClass shouldn't need to be involved at all. All tests would stay with PropTypes.

Basically, if I were to call React.PropTypes.oneOf('foo') outside of any React component, I should see a warning. This would ensure that contextTypes would also warn.

[zpao]

Since this is the example I keep using, let's work through it.

1. We put if (__DEV__) { warning(false, 'Some message here') }
2. Continue returning a chainable type checker (because isRequired can still be added).
3. We can perhaps just make it so that we pass it an empty function in or we keep it returning an error but we change that message to make it clear that this prop failure can be ignored if you are not the component author. I'm leaning towards the former right this second (because it's not terribly useful to have to work past a failure you aren't responsible for).

[rppc]

I think I get the idea!

[rppc]

@zpao Thanks for the review!

Like I said, this is my first contribution to React, so I apologise if I missed some important details.

What you said made sense. I will try to look into it when I have the chance.

Are we supposed to just log a warning in case the PropType is found to be invalid?

[facebook-github-bot]

@rppc updated the pull request.

[facebook-github-bot]

@rppc updated the pull request.

[facebook-github-bot]

@rppc updated the pull request.

[facebook-github-bot]

@rppc updated the pull request.

[rppc]

@zpao Is this what you had in mind? If so, I did over-complicate things a bit...

I'm so sorry... I accidentally merged all those unnecessary commits back in... Please tell me if should clean them up and I'll go on and just create a new pull request.

Two unit tests are failing because they expect the calls to ReactPropTypes functions not to warn. I've decided not to change those just yet.

[facebook-github-bot]

@rppc updated the pull request.

[zpao]

Please don't make another PR, just clean up your branch (strip that merge commit and don't run git merge master again (and/or fix your master branch)). It's important to use a singe PR so that we can track review comments and discussions. Looking across 10 PRs for that is super difficult, especially when you do it a year or 2 from now.

Please use the local environment to run tests and don't use Travis CI. Since we have a comment come in for every change, it gets noisy. npm test will run the tests.

Otherwise yes, this is on the right track for what I meant.

[rppc]

Yes, I shouldn't have merged that master branch... Silly me! I think I've got it now!

I'll fix the tests that are failing as soon as I can.

[facebook-github-bot]

@rppc updated the pull request.

[rppc]

@zpao As promised, I fixed the tests that were expecting functions React.PropTypes.oneOf() and React.PropTypes.oneOfType() not to warn and also some of the code so that all tests would pass.

[facebook-github-bot]

@rppc updated the pull request.

[facebook-github-bot]

@rppc updated the pull request.

[facebook-github-bot]

@rppc updated the pull request.

[zpao]

No need to do this, spies are mock functions. And no need to assign to a temporary variable and back - spies are cleaned up automatically. Here's a complete & short example usage: https://github.com/facebook/react/blob/master/src/addons/__tests__/ReactFragment-test.js#L93-L100

[rppc]

@zpao Done!

[facebook-github-bot]

@rppc updated the pull request.

[jimfb]

Ping @zpao

[gaearon]

Hey @rppc! We are sorry this got under our radar and doesn’t merge cleanly now. Someone else took a stab at it in #6316 which looks fairly similar so we’ll probably go ahead with it. Thank you for taking the time to contribute, and sorry for spending your time on the back-and-forth without merging the result.

[rppc]

@gaearon That's OK!