Skip to content
This repository has been archived by the owner on Sep 1, 2024. It is now read-only.

Support validation when hasOwnProperty is not in prototype #187

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Support validation when hasOwnProperty is not in prototype #187

merged 1 commit into from
Feb 7, 2019

Conversation

dferber90
Copy link
Contributor

@dferber90 dferber90 commented May 29, 2018

Avoids the Warning: Failed prop type: propValue.hasOwnProperty is not a function warning when hasOwnProperty is not defined in the prototype chain.

See #183 for details.


Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests with a null object, and an object with a hasOwnProperty property, would be great.

@@ -28,7 +28,7 @@ if (process.env.NODE_ENV !== 'production') {
function checkPropTypes(typeSpecs, values, location, componentName, getStack) {
if (process.env.NODE_ENV !== 'production') {
for (var typeSpecName in typeSpecs) {
if (typeSpecs.hasOwnProperty(typeSpecName)) {
if (Object.prototype.hasOwnProperty.call(typeSpecs, typeSpecName)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next robustness step is to do var has = Object.prototypr.hasOwnProperty at module scope, and then here, do has.call.

@dferber90
Copy link
Contributor Author

I moved the declaration to module scope now (amended the commit) and added a test for objects with a hasOwnProperty property.

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

@mhuggins
Copy link

mhuggins commented Feb 1, 2019

Thanks for this fix! Running into this as well. Is there a reason it sits open?

@dferber90
Copy link
Contributor Author

@mhuggins It is ready to be merged from my side (aside from the merge conflicts now).

@ljharb
Copy link
Contributor

ljharb commented Feb 7, 2019

Rebased this; it's partially addressed by #112 but still includes tests and a fix.

@ljharb ljharb merged commit 33e559c into facebook:master Feb 7, 2019
@mhuggins
Copy link

mhuggins commented Feb 7, 2019

Thanks, guys! ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants