Skip to content
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

Warn when multiple arguments were supplied to oneOf #244

Merged
merged 1 commit into from Feb 10, 2019
Merged

Warn when multiple arguments were supplied to oneOf #244

merged 1 commit into from Feb 10, 2019

Conversation

wojtekmaj
Copy link
Contributor

Adds a different warning message for multiple arguments supplied to oneOf. A common mistake is to write oneOf(x, y, z) instead of oneOf([x, y, z]) and this should help developers identifying the error.

Adds a different warning message for multiple arguments supplied to oneOf. A common mistake is to write oneOf(x, y, z) instead of oneOf([x, y, z]) and this should help developers identifying the error.
@@ -825,6 +825,20 @@ describe('PropTypesDevelopmentReact15', () => {
typeCheckPass(PropTypes.oneOf('red', 'blue'), 'red');
});

it('should warn but not error for invalid multiple arguments', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it better to warn here, and not throw an error? Wouldn’t that surface it most rapidly?

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 totally agree - it's my first contribution to prop-types so I'm being cautious here. I didn't want the output of neither oneOf('red') nor oneOf('red', 'blue') to change, as this may be considered breaking by some. So I just "enhanced" an error message in one of the cases.

__tests__/PropTypesDevelopmentReact15.js Outdated Show resolved Hide resolved
__tests__/PropTypesDevelopmentStandalone-test.js Outdated Show resolved Hide resolved
factoryWithTypeCheckers.js Outdated Show resolved Hide resolved
factoryWithTypeCheckers.js Outdated Show resolved Hide resolved
factoryWithTypeCheckers.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@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!

@wojtekmaj
Copy link
Contributor Author

Thanks for reviewing @ljharb. Force pushed to squash and change the unit tests to remove "instance of" from the strings you've changed.

@ljharb ljharb merged commit b4c8170 into facebook:master Feb 10, 2019
@wojtekmaj wojtekmaj deleted the feature/warn-for-multiple-oneOf-arguments branch February 10, 2019 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants