-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Support read-only props in Flow for no-unused-prop-types #1390
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
Changes from 5 commits
217d33e
cf5e8a3
c7c7191
2fcba06
7f1c9b3
44f03b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,10 +333,7 @@ module.exports = { | |
* @param {string} the identifier to strip | ||
*/ | ||
function stripQuotes(string) { | ||
if (string[0] === '\'' || string[0] === '"' && string[0] === string[string.length - 1]) { | ||
return string.slice(1, string.length - 1); | ||
} | ||
return string; | ||
return string.replace(/^\'|\'$/g, ''); | ||
} | ||
|
||
/** | ||
|
@@ -346,11 +343,8 @@ module.exports = { | |
*/ | ||
function getKeyValue(node) { | ||
if (node.type === 'ObjectTypeProperty') { | ||
const tokens = context.getFirstTokens(node, 2); | ||
return (tokens[0].value === '+' || tokens[0].value === '-' | ||
? tokens[1].value | ||
: stripQuotes(tokens[0].value) | ||
); | ||
const tokens = context.getFirstTokens(node, {count: 1, filter: tokenNode => ['Identifier', 'String'].indexOf(tokenNode.type) >= 0}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why would we want to strip all prefixes? only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a much safer implementation for the future. If Flow would introduce a new prefix, it should be handled automatically. (assuming that the prefix token type is similar to the types it has now). If you think keeping a list of prefixes is better then we can go with that approach as well... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to have explicit support; I'd rather it not silently risk doing the wrong thing when an unanticipated prefix exists. I definitely don't want the plugin to crash when a new prefix is found - but i might want it to warn on the prop. |
||
return stripQuotes(tokens[0].value); | ||
} | ||
const key = node.key || node.argument; | ||
return key.type === 'Identifier' ? key.name : key.value; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2032,27 +2032,50 @@ ruleTester.run('no-unused-prop-types', rule, { | |
}, { | ||
// issue #106 | ||
code: ` | ||
import React from 'react'; | ||
import SharedPropTypes from './SharedPropTypes'; | ||
import React from 'react'; | ||
import SharedPropTypes from './SharedPropTypes'; | ||
|
||
export default class A extends React.Component { | ||
render() { | ||
return ( | ||
<span | ||
a={this.props.a} | ||
b={this.props.b} | ||
c={this.props.c}> | ||
{this.props.children} | ||
</span> | ||
); | ||
export default class A extends React.Component { | ||
render() { | ||
return ( | ||
<span | ||
a={this.props.a} | ||
b={this.props.b} | ||
c={this.props.c}> | ||
{this.props.children} | ||
</span> | ||
); | ||
} | ||
} | ||
} | ||
|
||
A.propTypes = { | ||
a: React.PropTypes.string, | ||
...SharedPropTypes // eslint-disable-line object-shorthand | ||
}; | ||
`, | ||
A.propTypes = { | ||
a: React.PropTypes.string, | ||
...SharedPropTypes // eslint-disable-line object-shorthand | ||
}; | ||
`, | ||
parser: 'babel-eslint' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reformatted this test - that's why it's in the PR... |
||
}, { | ||
// issue #933 | ||
code: ` | ||
type Props = { | ||
+foo: number | ||
} | ||
class MyComponent extends React.Component { | ||
render() { | ||
return <div>{this.props.foo}</div> | ||
} | ||
} | ||
`, | ||
parser: 'babel-eslint' | ||
}, { | ||
code: ` | ||
type Props = { | ||
\'completed?\': boolean, | ||
}; | ||
const Hello = (props: Props): React.Element => { | ||
return <div>{props[\'completed?\']}</div>; | ||
} | ||
`, | ||
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.
can this repeated function be extracted out into a shared file?
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.
Yeah - that's what I mentioned in the other PR:
prop-types
andno-unused-prop-types
on first sight seem to share quite a bit of logic that deals with extracting the declared prop types for a component. And currently, the logic is not in sync (e.g. props in TypedArgument for Flow is not supported at all forno-unused-prop-types
.I'll try to deal with extracting as much of the shared logic as possible this weekend. Created an issue: #1393