Documentation for arrayOf custom item validator

[MatthewHerbst]

The docs currently show how to use a custom validator for a prop. However, it fails to show that this can be extended to doing custom validation on items within an array by passing a custom validator to arrayOf. This change makes that clear, as well as shows that the method signature for the custom validator passed into arrayOf is different versus just validating a prop.

// Signature for custom typechecker
function(props, propName, componentName) {}

// Signature for custom typechecker passed to arrayOf
function(arr, i, componentName, location, propFullName) {}

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

@MatthewHerbst updated the pull request.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[gaearon]

Hi, thanks for the PR! I would like to amend this a little bit.

The important part is not that arrayOf accepts a custom type checker—it’s that it accepts a PropType which is, after all, a function. So it’s not really specific to arrayOf. The same could be said about objectOf, for example.

The prop type function signature that arrayOf accepts is not really different from the “normal” one as far as I know. This is the reason you can pass any normal PropType there—precisely because it expects a PropType as an argument.

We are cheating a little bit by passing (array, index) instead of (props, propName) as the first argument so that we can keep them composable. However ultimately the first two arguments are “thing from which to read” (usually props but can be array) and “the key to read” (usually propName but can be index).

So while I think your edits are useful, it would help to clarify it arrayOf doesn’t really accept a function with a different signature than the existing prop types. The same is the case for the last two arguments (location and propFullName)—while they are not currently documented, they are passed both to top-level propTypes and to arrayOf or objectOf “child” propTypes so listing them only in the latter case seems misleading to me. I would probably omit them completely from the docs if the example doesn’t use them.

Sorry it took a long time to review, and thank you for contributing! I’ll be happy to review an update, if you’d like to keep working on this. For now, marking as needing an update.

[facebook-github-bot]

@MatthewHerbst updated the pull request.

[MatthewHerbst]

@gaearon Thanks for getting back to this. Actually, the entire reason I made the PR was to highlight the (array, index) part of the method signature, which is different compared to the normal custom validator.

I used that knowledge to write a custom validator that validates the format of the keys within each object of the array, rather than the values of the array. The original SO post that I asked might give you more context. Based on my knowledge and the knowledge within that SO post, the only way to individual object key validation when the keys are dynamic is to know that you have access to the object's container, which in this case is the array, and then access the object directly through that container (via the index). Is there a better way to do that where you don't need to know about (array, index)?

[gaearon]

Did objectOf() not satisfy your use case?
It’s mentioned in http://stackoverflow.com/a/35791125/458193 too.

Whoops, I get it now.
You want to check the keys (which are always strings in JS) for conformity to a specific format.

[gaearon]

As for your question—I would probably write something like objectWithKeysOf(childValidator) that works just like objectOf(childValidator) but runs the customValidator for the keys instead. It would receive the array of keys and the current index as the first two arguments. This way you could use an API similar to React’s existing API: MyPropTypes.objectWithKeysOf(MyPropTypes.stringRepresentingDate). Of course the approach you came to in the SO answer also works—I’m just suggesting making the child validator an argument, just like it is for arrayOf() and objectOf() so you can potentially pass different child validators there.

As for your PR—Let’s just remove the two last arguments? They are not used in the example and distract from the main point.

[gaearon]

Nit: Can we make a linebreak before the first The so it starts on the next line?
Another nit: on each time => for each item.
Yet another nit: please change , The to . and start The on the next line it doesn’t “hang” here either.

Thank you!

[facebook-github-bot]

@MatthewHerbst updated the pull request.

[MatthewHerbst]

@gaearon per your comments, I've reworked it a little bit.

• I've updated the example message in both custom examples to match as closely as possible the format found in the native error messages.
• I've reworked the description to make it clear that this applies to both arrayOf and objectOf, and have made the example generic enough to fit that.
• I kept the location, propFullName part because propFullName is actually very useful for error messages when working with a nested prop. Further, the non-object custom validator doesn't actually get passed a fullPropName (which makes sense since in that case propName == fullPropName since it isn't nested).

Let me know your thoughts.

[gaearon]

While we started using ES6 in the docs, I think we try to call it out explicitly when we do this (e.g. when we use classes and arrows). In this particular case I think we’re better off concatenating strings the traditional way so people who aren’t entirely comfortable with ES6 don’t get distracted by this.

[gaearon]

👍 This is looking good. I left a few nits and it’s ready to merge then.

[facebook-github-bot]

@MatthewHerbst updated the pull request.

[gaearon]

Please add a space between if and (

[facebook-github-bot]

@MatthewHerbst updated the pull request.

[gaearon]

Looks great! Can you please squash these into a single commit?
If you don’t feel comfortable doing it please let me know and I can do this manually on my side.

[MatthewHerbst]

@gaearon I think I might have done that slightly wrong - how to I avoid having to re-merge into myself after the rebase?

[gaearon]

I usually do something like this:

git checkout mybranch
git fetch origin // assuming 'facebook' is your origin
git rebase origin/master

// fix conflicts if any
// make sure rebase is over and git status says "N commits ahead of master"

git reset --soft origin/master // remove all commits
git commit -am 'my feature'
git push -f // overwrite PR

There are probably simpler ways but that is what I do.

[MatthewHerbst]

@gaearon That did it! Thanks 👍

[gaearon]

Thanks for sticking through with this!
The changes will appear on the website next time we sync (likely within a week or so).