Skip to content

Fix issues with IntersectionTypeAnnotation for prop-types and no-unused-prop-types #1415

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

Merged
merged 9 commits into from
Sep 12, 2017
Merged

Fix issues with IntersectionTypeAnnotation for prop-types and no-unused-prop-types #1415

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
87aca04
Add support for multiple Flow IntersectionTypeAnnotations and skip pr…
jseminck Sep 6, 2017
a771125
Add similar supprt for no-used-prop-types
jseminck Sep 6, 2017
20f75ee
Add support for TypeA & { ... }
Sep 6, 2017
19a2911
Clean up the code a bit
Sep 6, 2017
3e4d184
Aadd support also for no-unused-prop-types
Sep 6, 2017
3e9aefe
Make buildTypeAnnotationDeclarationTypes always return an object inst…
Sep 7, 2017
0ee5e93
Rewrite reduce to ignorePropsValidation as some()
Sep 7, 2017
357dcda
buildReactDeclarationTypes to always return an object
Sep 7, 2017
09c4ed7
Add tests for {...} & Props in addition to Props & {...}
jseminck Sep 8, 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
98 changes: 66 additions & 32 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 @@ -724,6 +724,70 @@ module.exports = {
});
}

/**
* Marks all props found inside ObjectTypeAnnotaiton as declared.
*
* Modifies the declaredProperties object
* @param {ASTNode} propTypes
* @param {Object} declaredPropTypes
* @returns {Boolean} True if propTypes should be ignored (e.g. when a type can't be resolved, when it is imported)
*/
function declarePropTypesForObjectTypeAnnotation(propTypes, declaredPropTypes) {
let ignorePropsValidation = false;

iterateProperties(propTypes.properties, (key, value) => {
if (!value) {
ignorePropsValidation = true;
return;
}

let types = buildTypeAnnotationDeclarationTypes(value, key);
if (types === true) {
Copy link
Member

Choose a reason for hiding this comment

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

instead of reusing types here, can we use multiple vars and use const?

Copy link
Contributor Author

@jseminck jseminck Sep 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is from the existing code so didn't touch it yet. But I think we can simply do:

const types = buildTypeAnnotationDeclarationTypes(value, key) || {};
declaredPropTypes.push({
  ...types,
  fullName: key,
  name: key,
  node: value
});

Edit: Ah! We don't support spread operator yet. 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meh, another problem is that buildTypeAnnotationDeclarationTypes returns either a boolean or an object which isn't great either. I try to refactor it to always return an object, but it needs changes in more places than just here.

But I do think it's a good refactor to do 😄

types = {};
}
types.fullName = key;
types.name = key;
types.node = value;
declaredPropTypes.push(types);
});

return ignorePropsValidation;
}

/**
* Marks all props found inside IntersectionTypeAnnotation as declared.
* Since InterSectionTypeAnnotations can be nested, this handles recursively.
*
* Modifies the declaredPropTypes object
* @param {ASTNode} propTypes
* @param {Object} declaredPropTypes
* @returns {Boolean} True if propTypes should be ignored (e.g. when a type can't be resolved, when it is imported)
*/
function declarePropTypesForIntersectionTypeAnnotation(propTypes, declaredPropTypes) {
Copy link
Member

Choose a reason for hiding this comment

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

Can these two functions live somewhere they can be shared across the rules that currently duplicate them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'm working on it 😄 The idea will be that the rule will be declared as:

create: propTypes.detect((context, components, utils) => {
...
})

And this will take care of ALL the shared code between prop-types and no-unused-prop-types. Perhaps also for the other rules.

return propTypes.types.reduce((ignorePropsValidation, annotation) => {
Copy link
Member

Choose a reason for hiding this comment

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

since this is reducing to a boolean, could it use some or every, and that way it will bail early?

// If we already decided to skip props validation then we don't have to continue processing anything else
if (ignorePropsValidation) {
return ignorePropsValidation;
}

if (annotation.type === 'ObjectTypeAnnotation') {
ignorePropsValidation = declarePropTypesForObjectTypeAnnotation(annotation, declaredPropTypes);
} else {
const typeNode = typeScope(annotation.id.name);

if (!typeNode) {
ignorePropsValidation = true;
} else if (typeNode.type === 'IntersectionTypeAnnotation') {
ignorePropsValidation = declarePropTypesForIntersectionTypeAnnotation(typeNode, declaredPropTypes);
} else {
ignorePropsValidation = declarePropTypesForObjectTypeAnnotation(typeNode, declaredPropTypes);
}
}

return ignorePropsValidation;
Copy link
Member

Choose a reason for hiding this comment

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

i can't decide if this style, or early-returning each place this var is assigned, is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I'm in general not really happy about the fact that a function with a name declarePropTypes*** returns a boolean at all. It's a bit confusing. That's why I liked this style a bit more, because it's very explicit to say what those methods return and what the boolean actually means.

Perhaps a try/catch may be even better?

Copy link
Member

Choose a reason for hiding this comment

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

We could rename the function instead :-)

}, false);
}

/**
* Mark a prop type as declared
* @param {ASTNode} node The AST node being checked.
Expand All @@ -736,20 +800,7 @@ module.exports = {

switch (propTypes && propTypes.type) {
case 'ObjectTypeAnnotation':
iterateProperties(propTypes.properties, (key, value) => {
if (!value) {
ignorePropsValidation = true;
return;
}
let types = buildTypeAnnotationDeclarationTypes(value, key);
if (types === true) {
types = {};
}
types.fullName = key;
types.name = key;
types.node = value;
declaredPropTypes.push(types);
});
ignorePropsValidation = declarePropTypesForObjectTypeAnnotation(propTypes, declaredPropTypes);
break;
case 'ObjectExpression':
iterateProperties(propTypes.properties, (key, value) => {
Expand Down Expand Up @@ -791,24 +842,7 @@ module.exports = {
}
break;
case 'IntersectionTypeAnnotation':
propTypes.types.forEach(annotation => {
const propsType = typeScope(annotation.id.name);
iterateProperties(propsType.properties, (key, value) => {
if (!value) {
ignorePropsValidation = true;
return;
}

let types = buildTypeAnnotationDeclarationTypes(value, key);
if (types === true) {
types = {};
}
types.fullName = key;
types.name = key;
types.node = value;
declaredPropTypes.push(types);
});
});
ignorePropsValidation = declarePropTypesForIntersectionTypeAnnotation(propTypes, declaredPropTypes);
break;
case null:
break;
Expand Down
75 changes: 58 additions & 17 deletions lib/rules/prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,62 @@ module.exports = {
});
}

/**
* Marks all props found inside ObjectTypeAnnotaiton as declared.
*
* Modifies the declaredProperties object
* @param {ASTNode} propTypes
* @param {Object} declaredPropTypes
* @returns {Boolean} True if propTypes should be ignored (e.g. when a type can't be resolved, when it is imported)
*/
function declarePropTypesForObjectTypeAnnotation(propTypes, declaredPropTypes) {
let ignorePropsValidation = false;

iterateProperties(propTypes.properties, (key, value) => {
if (!value) {
ignorePropsValidation = true;
return;
}
declaredPropTypes[key] = buildTypeAnnotationDeclarationTypes(value);
});

return ignorePropsValidation;
}

/**
* Marks all props found inside IntersectionTypeAnnotation as declared.
* Since InterSectionTypeAnnotations can be nested, this handles recursively.
*
* Modifies the declaredPropTypes object
* @param {ASTNode} propTypes
* @param {Object} declaredPropTypes
* @returns {Boolean} True if propTypes should be ignored (e.g. when a type can't be resolved, when it is imported)
*/
function declarePropTypesForIntersectionTypeAnnotation(propTypes, declaredPropTypes) {
return propTypes.types.reduce((ignorePropsValidation, annotation) => {
// If we already decided to skip props validation then we don't have to continue processing anything else
if (ignorePropsValidation) {
return ignorePropsValidation;
}

if (annotation.type === 'ObjectTypeAnnotation') {
ignorePropsValidation = declarePropTypesForObjectTypeAnnotation(annotation, declaredPropTypes);
} else {
const typeNode = typeScope(annotation.id.name);

if (!typeNode) {
ignorePropsValidation = true;
} else if (typeNode.type === 'IntersectionTypeAnnotation') {
ignorePropsValidation = declarePropTypesForIntersectionTypeAnnotation(typeNode, declaredPropTypes);
} else {
ignorePropsValidation = declarePropTypesForObjectTypeAnnotation(typeNode, declaredPropTypes);
}
}

return ignorePropsValidation;
}, false);
}

/**
* Mark a prop type as declared
* @param {ASTNode} node The AST node being checked.
Expand All @@ -726,13 +782,7 @@ module.exports = {

switch (propTypes && propTypes.type) {
case 'ObjectTypeAnnotation':
iterateProperties(propTypes.properties, (key, value) => {
if (!value) {
ignorePropsValidation = true;
return;
}
declaredPropTypes[key] = buildTypeAnnotationDeclarationTypes(value);
});
ignorePropsValidation = declarePropTypesForObjectTypeAnnotation(propTypes, declaredPropTypes);
break;
case 'ObjectExpression':
iterateProperties(propTypes.properties, (key, value) => {
Expand Down Expand Up @@ -793,16 +843,7 @@ module.exports = {
}
break;
case 'IntersectionTypeAnnotation':
propTypes.types.forEach(annotation => {
const propsType = typeScope(annotation.id.name);
iterateProperties(propsType.properties, (key, value) => {
if (!value) {
ignorePropsValidation = true;
return;
}
declaredPropTypes[key] = buildTypeAnnotationDeclarationTypes(value);
});
});
ignorePropsValidation = declarePropTypesForIntersectionTypeAnnotation(propTypes, declaredPropTypes);
break;
case null:
break;
Expand Down
88 changes: 88 additions & 0 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 @@ -839,6 +839,55 @@ ruleTester.run('no-unused-prop-types', rule, {
}
`,
parser: 'babel-eslint'
}, {
code: `
type PropsA = { foo: string };
type PropsB = { bar: string };
type PropsC = { zap: string };
type Props = PropsA & PropsB;

class Bar extends React.Component {
props: Props & PropsC;

render() {
return <div>{this.props.foo} - {this.props.bar} - {this.props.zap}</div>
}
}
`,
parser: 'babel-eslint'
}, {
code: `
import type { PropsA } from "./myPropsA";
type PropsB = { bar: string };
type PropsC = { zap: string };
type Props = PropsA & PropsB;

class Bar extends React.Component {
props: Props & PropsC;

render() {
return <div>{this.props.foo} - {this.props.bar} - {this.props.zap}</div>
}
}
`,
parser: 'babel-eslint'
}, {
code: `
type PropsB = { foo: string };
type PropsC = { bar: string };
type Props = PropsB & {
zap: string
};

class Bar extends React.Component {
props: Props & PropsC;

render() {
return <div>{this.props.foo} - {this.props.bar} - {this.props.zap}</div>
}
}
`,
parser: 'babel-eslint'
}, {
code: [
'import type Props from "fake";',
Expand Down Expand Up @@ -2743,6 +2792,45 @@ ruleTester.run('no-unused-prop-types', rule, {
errors: [
{message: '\'b\' PropType is defined but prop is never used'}
]
}, {
code: `
type PropsA = { foo: string };
type PropsB = { bar: string };
type PropsC = { zap: string };
type Props = PropsA & PropsB;

class Bar extends React.Component {
props: Props & PropsC;

render() {
return <div>{this.props.foo} - {this.props.bar}</div>
}
}
`,
parser: 'babel-eslint',
errors: [
{message: '\'zap\' PropType is defined but prop is never used'}
]
}, {
code: `
type PropsB = { foo: string };
type PropsC = { bar: string };
type Props = PropsB & {
zap: string
};

class Bar extends React.Component {
props: Props & PropsC;

render() {
return <div>{this.props.foo} - {this.props.bar}</div>
}
}
`,
errors: [
{message: '\'zap\' PropType is defined but prop is never used'}
],
parser: 'babel-eslint'
}, {
code: [
'class Hello extends React.Component {',
Expand Down
Loading