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

no-unused-props: not ignoring components with no used props #1303

Merged
merged 3 commits into from
Jul 21, 2017
Merged

no-unused-props: not ignoring components with no used props #1303

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d0707e
no ignoring components with no used props
Jul 14, 2017
430a89f
moving test into invalid
Jul 16, 2017
64e837b
Merge branch 'master' into noUsedProps
Jul 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions lib/rules/no-unused-prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ module.exports = {
function mustBeValidated(component) {
return Boolean(
component &&
component.usedPropTypes &&
!component.ignorePropsValidation
);
}
Expand Down Expand Up @@ -234,8 +233,9 @@ module.exports = {
* @returns {Boolean} True if the prop is used, false if not.
*/
function isPropUsed(node, prop) {
for (let i = 0, l = node.usedPropTypes.length; i < l; i++) {
const usedProp = node.usedPropTypes[i];
const usedPropTypes = node.usedPropTypes || [];
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 reverted that change from Components file.

for (let i = 0, l = usedPropTypes.length; i < l; i++) {
const usedProp = usedPropTypes[i];
if (
prop.type === 'shape' ||
prop.name === '__ANY_KEY__' ||
Expand Down Expand Up @@ -897,6 +897,13 @@ module.exports = {
}
},

JSXSpreadAttribute: function(node) {
const component = components.get(utils.getParentComponent());
Copy link
Member

Choose a reason for hiding this comment

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

It seems super weird to me that getParentComponent infers the node rather than taking it as an argument (not your code ofc, just thought I'd comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap

components.set(component ? component.node : node, {
ignorePropsValidation: true
});
},

MethodDefinition: function(node) {
if (!isPropTypesDeclaration(node.key)) {
return;
Expand Down
83 changes: 15 additions & 68 deletions tests/lib/rules/no-unused-prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,26 +557,6 @@ ruleTester.run('no-unused-prop-types', rule, {
'});'
].join('\n'),
options: [{customValidators: ['CustomValidator']}]
}, {
code: [
'class Comp1 extends Component {',
' render() {',
' return <span />;',
' }',
'}',
'Comp1.propTypes = {',
' prop1: PropTypes.number',
'};',
'class Comp2 extends Component {',
' render() {',
' return <span />;',
' }',
'}',
'Comp2.propTypes = {',
' prop2: PropTypes.arrayOf(Comp1.propTypes.prop1)',
Copy link
Member

Choose a reason for hiding this comment

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

This test implies that Comp1's prop1 prop should be marked as used, because Comp2 references its propType; but in fact I think it shouldn't - could you move this test to be invalid instead of just deleting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

'};'
].join('\n'),
parser: 'babel-eslint'
}, {
code: [
'const SomeComponent = createReactClass({',
Expand Down Expand Up @@ -782,54 +762,6 @@ ruleTester.run('no-unused-prop-types', rule, {
' }',
'});'
].join('\n')
}, {
code: [
'const statelessComponent = (props) => {',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

First of all for some reason the same test were copy-pasted 4 times. I had to remove this test all together because it was never validated correctly. Current implementation is not able to detect correctly nested definition of components (subRender technically is a react component). I don't think this is a common/idiomatic react style so I assume we shouldn't support it.

Copy link
Member

Choose a reason for hiding this comment

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

It's not the same test; I see 4 different variations: arrow, non-arrow, and destructured, and non-destructured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Makes sense. Is ti ok if I delete these tests though. Do you agree that this rule technically doesn't support this use case.

Copy link
Member

Choose a reason for hiding this comment

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

I think that it ideally should support this use case, but our mechanism of detecting components may make it difficult.

Copy link

Choose a reason for hiding this comment

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

If you ask me, the fact that certain cases are not supported could also be tested. Of course, with a test name which clearly states "testing unsupported case" and assertions that clearly check for false positives or other signs of "not supported"-ness.

Copy link
Member

Choose a reason for hiding this comment

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

sure, that seems reasonable too.

' const subRender = () => {',
' return <span>{props.someProp}</span>;',
' };',
' return <div>{subRender()}</div>;',
'};',
'statelessComponent.propTypes = {',
' someProp: PropTypes.string',
'};'
].join('\n')
}, {
code: [
'const statelessComponent = ({ someProp }) => {',
' const subRender = () => {',
' return <span>{someProp}</span>;',
' };',
' return <div>{subRender()}</div>;',
'};',
'statelessComponent.propTypes = {',
' someProp: PropTypes.string',
'};'
].join('\n')
}, {
code: [
'const statelessComponent = function({ someProp }) {',
' const subRender = () => {',
' return <span>{someProp}</span>;',
' };',
' return <div>{subRender()}</div>;',
'};',
'statelessComponent.propTypes = {',
' someProp: PropTypes.string',
'};'
].join('\n')
}, {
code: [
'function statelessComponent({ someProp }) {',
' const subRender = () => {',
' return <span>{someProp}</span>;',
' };',
' return <div>{subRender()}</div>;',
'};',
'statelessComponent.propTypes = {',
' someProp: PropTypes.string',
'};'
].join('\n')
}, {
code: [
'function notAComponent({ something }) {',
Expand Down Expand Up @@ -3215,6 +3147,21 @@ ruleTester.run('no-unused-prop-types', rule, {
line: 11,
column: 8
}]
}, { // None of the props are used issue #1162
code: [
'import React from "react"; ',
'var Hello = React.createReactClass({',
' propTypes: {',
' name: React.PropTypes.string',
' },',
' render: function() {',
' return <div>Hello Bob</div>;',
' }',
'});'
].join('\n'),
errors: [{
message: '\'name\' PropType is defined but prop is never used'
}]
}
/* , {
// Enable this when the following issue is fixed
Expand Down