-
Notifications
You must be signed in to change notification settings - Fork 46.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove prop types checking in ReactCompositeComponent #6824
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,7 +197,7 @@ var ReactCompositeComponentMixin = { | |
this._hostParent = hostParent; | ||
this._hostContainerInfo = hostContainerInfo; | ||
|
||
var publicProps = this._processProps(this._currentElement.props); | ||
var publicProps = this._currentElement.props; | ||
var publicContext = this._processContext(context); | ||
|
||
var Component = this._currentElement.type; | ||
|
@@ -614,7 +614,7 @@ var ReactCompositeComponentMixin = { | |
if (__DEV__) { | ||
var Component = this._currentElement.type; | ||
if (Component.contextTypes) { | ||
this._checkPropTypes( | ||
this._checkContextTypes( | ||
Component.contextTypes, | ||
maskedContext, | ||
ReactPropTypeLocations.context | ||
|
@@ -647,7 +647,7 @@ var ReactCompositeComponentMixin = { | |
this.getName() || 'ReactCompositeComponent' | ||
); | ||
if (__DEV__) { | ||
this._checkPropTypes( | ||
this._checkContextTypes( | ||
Component.childContextTypes, | ||
childContext, | ||
ReactPropTypeLocations.childContext | ||
|
@@ -667,39 +667,14 @@ var ReactCompositeComponentMixin = { | |
}, | ||
|
||
/** | ||
* Processes props by setting default values for unspecified props and | ||
* asserting that the props are valid. Does not mutate its argument; returns | ||
* a new props object with defaults merged in. | ||
* Assert that the context types are valid | ||
* | ||
* @param {object} newProps | ||
* @return {object} | ||
* @private | ||
*/ | ||
_processProps: function(newProps) { | ||
if (__DEV__) { | ||
var Component = this._currentElement.type; | ||
if (Component.propTypes) { | ||
this._checkPropTypes( | ||
Component.propTypes, | ||
newProps, | ||
ReactPropTypeLocations.prop | ||
); | ||
} | ||
} | ||
return newProps; | ||
}, | ||
|
||
/** | ||
* Assert that the props are valid | ||
* | ||
* @param {object} propTypes Map of prop name to a ReactPropType | ||
* @param {object} propTypes Map of context field to a ReactPropType | ||
* @param {object} props | ||
* @param {string} location e.g. "prop", "context", "child context" | ||
* @private | ||
*/ | ||
_checkPropTypes: function(propTypes, props, location) { | ||
// TODO: Stop validating prop types here and only use the element | ||
// validation. | ||
_checkContextTypes: function(propTypes, props, location) { | ||
var componentName = this.getName(); | ||
for (var propName in propTypes) { | ||
if (propTypes.hasOwnProperty(propName)) { | ||
|
@@ -724,23 +699,12 @@ var ReactCompositeComponentMixin = { | |
// top-level render calls, so I'm abstracting it away into | ||
// a function to minimize refactoring in the future | ||
var addendum = getDeclarationErrorAddendum(this); | ||
|
||
if (location === ReactPropTypeLocations.prop) { | ||
// Preface gives us something to blacklist in warning module | ||
warning( | ||
false, | ||
'Failed Composite propType: %s%s', | ||
error.message, | ||
addendum | ||
); | ||
} else { | ||
warning( | ||
false, | ||
'Failed Context Types: %s%s', | ||
error.message, | ||
addendum | ||
); | ||
} | ||
warning( | ||
false, | ||
'Failed Context Types: %s%s', | ||
error.message, | ||
addendum | ||
); | ||
} | ||
} | ||
} | ||
|
@@ -824,13 +788,10 @@ var ReactCompositeComponentMixin = { | |
willReceive = true; | ||
} | ||
|
||
// Distinguish between a props update versus a simple state update | ||
if (prevParentElement === nextParentElement) { | ||
// Skip checking prop types again -- we don't read inst.props to avoid | ||
// warning for DOM component props in this upgrade | ||
nextProps = nextParentElement.props; | ||
} else { | ||
nextProps = this._processProps(nextParentElement.props); | ||
nextProps = nextParentElement.props; | ||
|
||
// Not a simple state update but a props update | ||
if (prevParentElement !== nextParentElement) { | ||
willReceive = true; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you simplify this to
now? |
||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind switching this to use getStackAddendumByID to show the stack? That can be a separate PR if you prefer.