Skip to content

Conversation

@mathieumg
Copy link
Contributor

The rationale behind this is when you have object literals used as dictionaries and you want to validate each property (without knowing its name ahead of time). PropTypes.shape allows to validate specific properties, but this rather iterates on the properties of an object (no matter their name) and validates their content (which then can be any of the PropTypes). It works very similarly to arrayOf, so I strongly inspired myself from its code both for the implementation and the tests.

If you're interested in merging this, I imagine the name can change but I had trouble coming up with something as straightforward and unambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

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

@spicyj @zpao I think we settled on using hasOwnProperty() in the rest of the React code base right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically you want me to wrap it with if (propValue.hasOwnProperty(key)) for each iteration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. The issue #1385 I was thinking of is still open it seems, but I believe we're mostly using hasOwnProperty elsewhere, I personally don't really mind either. Can wait for @zpao @spicyj or whoever to weigh in if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind adding it to be on the "safe side", especially if it's already the approach. Prop validation is dev-only anyway so that's negligible overhead.

@syranide
Copy link
Contributor

It seems like a sound idea and the code looks great to me.
👍 Good job!

@zpao
Copy link
Member

zpao commented May 27, 2014

Interestingly enough, we just had somebody work on almost exactly this on Friday internally (hi @dschafer!). I wasn't sure if we really wanted to support it in core since it's a bit specialized, but perhaps I was wrong...

@sophiebits
Copy link
Collaborator

We also wanted this at KA two weeks ago.

For naming, I was thinking perhaps map or dict. Using map is nice in that it matches the ES6 Map, but perhaps also confusing for the same reason.

@chenglou
Copy link
Contributor

👍
One day ReactPropTypes will reach feature-parity with Haskell's type system (jk).

@zpao
Copy link
Member

zpao commented May 27, 2014

The version we had internally used the name dictOf but I actually think objectOf is the best fit. It follows arrayOf and says object which matches the type we already have.

I don't have any code comments, so I'm going to go ahead and pull this in.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: please remove trailing whitespace here

@sebmarkbage
Copy link
Collaborator

IMO, using objects as dictionaries should be highly discouraged. You should be using an ES6 Map and we could add an mapOf function.

@vjeux
Copy link
Contributor

vjeux commented Jun 13, 2014

@sebmarkbage no one is going to be able to use ES6 Map in a long time. I think that it's worth supporting objectOf in the meantime.

@sebmarkbage
Copy link
Collaborator

@vjeux We'll require a shim of ES6 Map in React core soon anyway. So everyone will be able to use it.

@chenglou
Copy link
Contributor

Internally merged. We can have both!

chenglou added a commit that referenced this pull request Jun 13, 2014
Added 'objectOf' PropType validator to iterate on objects and validate properties.
@chenglou chenglou merged commit 4329d5a into facebook:master Jun 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants