-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix nextProps false positive in no-unused-prop-types #1106
Conversation
lib/rules/no-unused-prop-types.js
Outdated
@@ -17,7 +17,7 @@ var annotations = require('../util/annotations'); | |||
// Constants | |||
// ------------------------------------------------------------------------------ | |||
|
|||
var DIRECT_PROPS_REGEX = /^props\s*(\.|\[)/; | |||
var DIRECT_PROPS_REGEX = /^(props|nextProps)\s*(\.|\[)/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to suggest that there will be places that props
and nextProps
are interchangeable - that doesn't seem right to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did exactly like the prop-types
rules. It means that this one is wrong too.
What do you suggest? This regex is only used one time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a try. I split the regex into 2 variables. It leads to more conditions. Tell me if you prefer this version.
' }', | ||
'}' | ||
].join('\n'), | ||
parser: 'babel-eslint' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no need to use babel-eslint here; let's just use normal Hello.propTypes =
, and use the default parser
1dfff04
to
6986798
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this looks much better. The prop-types rule could probably stand to be updated in the same way, but that's a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
+1 |
Fixes #1079
I did the same comparisons as the
prop-types
rules (https://github.com/yannickcr/eslint-plugin-react/blob/b64648521f9a7949eef4cd1a210768e906b4f3d1/lib/rules/prop-types.js#L130) for theno-unused-prop-types
rule.This way they both handle nextProps identically.
I also borrowed the test case from #1105.