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

New: Add require-default-props rule (fixes #528) #965

Merged
merged 6 commits into from
Dec 3, 2016

Conversation

vitorbal
Copy link
Contributor

@vitorbal vitorbal commented Nov 19, 2016

Implements the new require-default-props rule. (#528)

This is a work in progress: support for flow annotations hasn't been added yet, but the code is already considerably big, so I thought I could get the PR up already to get some early feedback on implementation, docs, missing test cases, etc. flow annotation support has now been added.

This is my first time contributing to eslint-plugin-react, so I hope I didn't do something horribly wrong in the implementation of the rule 😟

Copy link
Member

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

It would be great to have some tests that show that defaulting SFC arguments in the signature won't suffice; as well as updating the example documentation to explicitly describe why default arguments aren't the same as defaultProps

@vitorbal
Copy link
Contributor Author

@ljharb thanks for the feedback! I've updated the docs to more explicitly mention that defaultProps are still preferred over default function params. Is this what you had in mind?

@ljharb
Copy link
Member

ljharb commented Nov 20, 2016

Your changes look great, thanks!

@@ -29,8 +29,7 @@ function findVariable(variables, name) {
* Contain a patch for babel-eslint to avoid https://github.com/babel/babel-eslint/issues/21
*
* @param {Object} context The current rule context.
* @param {Array} name The name of the variable to search.
* @returns {Boolean} True if the variable was found, false if not.
* @returns {Array} The variables list
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope this is okay, sneaked in a fix for the wrong jsdoc for this function.

@vitorbal
Copy link
Contributor Author

@ljharb @yannickcr alright, i pushed some new commits that add support for flow annotations to the rule. I tried to come up with the biggest number of test cases I could think of, but unfortunately I don't have any big repositories that use react and flow to test the rule against.

Are there any opensource projects that use both react and flow that we could test the rule against to catch some potential bugs? @ljharb is there any chance you could give the rule a test-drive at Airbnb?

@ljharb
Copy link
Member

ljharb commented Nov 25, 2016

@vitorbal Airbnb does not yet use Flow; I don't have any projects I can test flow support on.

@vitorbal
Copy link
Contributor Author

vitorbal commented Dec 1, 2016

I added some extra tests for flow annotation use cases. Unfortunately I couldn't find any codebases using flow to test the rule on. However, I'm willing to react fast to any bugs found after this rule gets published, FWIW.

@ljharb ljharb added the new rule label Dec 1, 2016
@yannickcr yannickcr merged commit e1abec9 into jsx-eslint:master Dec 3, 2016
@yannickcr
Copy link
Member

This is really great, thanks!

@vitorbal vitorbal deleted the require-default-props branch December 7, 2016 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants