Skip to content

Validating PropTypes when a class is created - #5475

Closed
rppc wants to merge 250 commits into
react:masterfrom
rppc:onlyCode
Closed

Validating PropTypes when a class is created#5475
rppc wants to merge 250 commits into
react:masterfrom
rppc:onlyCode

Conversation

@rppc

@rppc rppc commented Nov 15, 2015

Copy link
Copy Markdown

Note: This is the same pull request, but I've had to switch branches.

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

AMARALD and others added 30 commits October 1, 2015 17:39
@zpao

zpao commented Nov 15, 2015

Copy link
Copy Markdown
Contributor

There are a lot of unrelated commits here. Can you clean that up?

@rppc

rppc commented Nov 15, 2015

Copy link
Copy Markdown
Author

Yes, sorry about that (and also the fact that they're written in another language)...

I'm not so sure how to do that smoothly... I have a branch in the forked repo where those commits are relevant. Could you possibly suggest a way for me to clean up only the branch which this pull request refers to?

@jimfb

jimfb commented Nov 15, 2015

Copy link
Copy Markdown
Contributor

@rppc In the branch you create for this pull request, you can do a git rebase -i 984c2610772054aedc3091ae9877a23f195b7a70 and then squash all the commits and do a git push -f to force update the branch. Alternatively, you could create a new fork/branch from github's react/master branch and cherry-pick your commit over.

@rppc

rppc commented Nov 15, 2015

Copy link
Copy Markdown
Author

@jimfb After rebasing, I ran 'git merge --squash' but got "(nothing to squash)Already up-to-date".

Perhaps I'm missing something...

@jimfb

jimfb commented Nov 15, 2015

Copy link
Copy Markdown
Contributor

git rebase -i allows you to specify which commits should be squashed as part of the rebase, so after the rebase everything would be squashed (if you specify squash during the rebase) - I'd suggest doing a google search for "interactive rebase squash". The only downside to this approach is that there are a whole ton of commits, which is why I suggested cherry-picking might be easier in this case. Either way, it's just a matter of fighting with git until you get it working - git is sometimes annoyingly complicated.

@rppc

rppc commented Nov 15, 2015

Copy link
Copy Markdown
Author

Definitely not the smoothest way to do this, but I've created a new branch where the commit history has been cleaned up. Please refer to this pull request.

@rppc rppc closed this Nov 15, 2015
@rppc
rppc deleted the onlyCode branch November 15, 2015 21:06
@zpao

zpao commented Nov 15, 2015

Copy link
Copy Markdown
Contributor

I have some instructions here that should be helpful with what I would have done. I know you just made a new PR to work from which is great but perhaps for the future

git remote add facebook git@github.com:facebook/react.git
git fetch facebook
git branch onlyCode2 --no-track facebook/master
git diff facebook/master onlyCode | patch -p1
# add and commit normally, this will be a standalone commit with only code
# git add
# git commit
# make a new PR

@rppc

rppc commented Nov 15, 2015

Copy link
Copy Markdown
Author

@zpao I ended up doing something similar, but it looks like I'll have to do it again because I accidentally let all those commits back in...

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants