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.

Sign up for GitHub

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

Jump to bottom

Fix false positive in no-unused-proptype #1218

Merged
merged 3 commits into from May 24, 2017
Merged

Fix false positive in no-unused-proptype #1218

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cea8591
Add failing and working tests that were raised in the issue
jseminck May 19, 2017
f85ff44
Fix issue with overriding component usedPropTypes if the component al…
jseminck May 22, 2017
bf69820
Replace var with const for the new code that was added
May 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/util/Components.js
View file
Open in desktop
Expand Up @@ -99,9 +99,12 @@ Components.prototype.list = function() {
component = this.get(node);
}
if (component) {
usedPropTypes[this._getId(component.node)] = (this._list[i].usedPropTypes || []).filter(function(propType) {
var newUsedProps = (this._list[i].usedPropTypes || []).filter(function(propType) {
Copy link
Member

Choose a reason for hiding this comment

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

s/var/const/g

Copy link
Contributor Author

@jseminck jseminck May 23, 2017 •
edited

Choose a reason for hiding this comment

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

Unfortunately it's not so simple in this file. There are many vars that cannot be changed to const.

Personally I also feel like a file should either use only var, or only let + const. Using a mix of var, let and const is meh.

If you want, I can make some separate PR and transform some of the code-base to ES6? I have used Lebab before (https://github.com/lebab/lebab). I just need to figure out what ES6 features are supported and which aren't. Although we could just start with getting rid of var completely?

Copy link
Member

Choose a reason for hiding this comment

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

I agree; we should only use const and let.

Regardless, we shouldn't add new instances of var - a mix is preferable to that.

I'd prefer manual modification to a codemod.

return !propType.node || propType.node.kind !== 'init';
});

var componentId = this._getId(component.node);
usedPropTypes[componentId] = (usedPropTypes[componentId] || []).concat(newUsedProps);
}
}
// Assign used props in not confident components to the parent component
Expand Down
45 changes: 45 additions & 0 deletions tests/lib/rules/no-unused-prop-types.js
View file
Open in desktop
Expand Up @@ -1439,6 +1439,51 @@ ruleTester.run('no-unused-prop-types', rule, {
'}'
].join('\n'),
parser: 'babel-eslint'
}, {
// The next two test cases are related to: https://github.com/yannickcr/eslint-plugin-react/issues/1183
code: [
'export default function SomeComponent(props) {',
' const callback = () => {',
' props.a(props.b);',
' };',
'',
' const anotherCallback = () => {};',
'',
' return (',
' <SomeOtherComponent',
' name={props.c}',
' callback={callback}',
' />',
' );',
'}',
'',
'SomeComponent.propTypes = {',
' a: React.PropTypes.func.isRequired,',
' b: React.PropTypes.string.isRequired,',
' c: React.PropTypes.string.isRequired,',
'};'
].join('\n')
}, {
code: [
'export default function SomeComponent(props) {',
' const callback = () => {',
' props.a(props.b);',
' };',
'',
' return (',
' <SomeOtherComponent',
' name={props.c}',
' callback={callback}',
' />',
' );',
'}',
'',
'SomeComponent.propTypes = {',
' a: React.PropTypes.func.isRequired,',
' b: React.PropTypes.string.isRequired,',
' c: React.PropTypes.string.isRequired,',
'};'
].join('\n')
}
],

Expand Down