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

Fix silently failing invalid validators in shape#234

Merged
ljharb merged 1 commit into
facebook:masterfrom
asbjornh:fix-invalid-validators-in-shape
Feb 27, 2019
Merged

Fix silently failing invalid validators in shape#234
ljharb merged 1 commit into
facebook:masterfrom
asbjornh:fix-invalid-validators-in-shape

Conversation

@asbjornh

@asbjornh asbjornh commented Oct 26, 2018

Copy link
Copy Markdown

Fixes #220
Possibly also #181

This PR adds type checks for validators in createShapeTypeChecker and createStrictShapeTypeChecker so that they can fail loudly on invalid validators and everyone can be safe and happy! 😊

As outlined in #220 , providing any falsy value instead of a validator will cause validation to be skipped, meaning invalid props won't be caught. Providing a truthy value that's not a function results in a caught but displayed javascript error Warning: Failed prop type: checker is not a function which is better than nothing but not really that helpful.

The most likely cause of issues is typos in validator names, like PropTypes.boolean (which I've since discovered that I've written several times). As mentioned in #220 this is mitigated by using eslint-plugin-react/no-typos, but I do feel that relying on a third party that's not a dependency is more of a band-aid than a solution.

Another cause would be references to internal or external things that might or might not be functions, which is not caught by the eslint plugin.

const validator = false;
Component.propTypes = {
  foo: PropTypes.shape({ bar: validator })
};

@ljharb ljharb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@ljharb

ljharb commented Feb 21, 2019

Copy link
Copy Markdown
Contributor

@gaearon do you think this is a bug fix, or a breaking change because it might introduce additional warnings?

@asbjornh

Copy link
Copy Markdown
Author

@ljharb In hindsight I think the tests might be a little thin. I'd be happy to add some more invalid cases in both tests unless that's too late now (I don't fully understand what you did with with those force pushes)

@ljharb

ljharb commented Feb 22, 2019

Copy link
Copy Markdown
Contributor

@asbjornh definitely not too late! i rebased the PR branch; after a git fetch locally you can git reset --hard origin/fix-invalid-validators-in-shape, or git checkout master && git branch -D fix-invalid-validators-in-shape && git checkout fix-invalid-validators-in-shape, or you can delete and re-clone your local repo.

@asbjornh

Copy link
Copy Markdown
Author

Great! Thanks :)

@asbjornh

Copy link
Copy Markdown
Author

@ljharb I've added some more test cases (which was good because I discovered an issue). I think this is ready now, unless you guys want any changes :)

Comment thread factoryWithTypeCheckers.js Outdated
Comment thread factoryWithTypeCheckers.js Outdated
@ljharb
ljharb merged commit 9e7afa3 into facebook:master Feb 27, 2019
@asbjornh

Copy link
Copy Markdown
Author

Yay!

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.

3 participants