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.

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 requireReadOnlyReactProps #406

Merged
merged 8 commits into from May 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions README.md
Expand Up @@ -1149,7 +1149,7 @@ import Foo from './foo';
// Message: Expected newline after flow annotation

// Options: ["always-windows"]
// @flow
// @flow
import Foo from './foo';
// Message: Expected newline after flow annotation

Expand All @@ -1169,8 +1169,8 @@ The following patterns are not considered problems:
import Foo from './foo';

// Options: ["always-windows"]
// @flow
// @flow

import Foo from './foo';

// Options: ["never"]
Expand Down Expand Up @@ -3701,7 +3701,7 @@ The following patterns are not considered problems:
{ a: string, b: number }) => {}

// Options: ["always",{"allowLineBreak":true}]
(foo:
(foo:
{ a: string, b: number }) => {}

// Options: ["never"]
Expand Down
35 changes: 23 additions & 12 deletions src/rules/requireReadonlyReactProps.js
@@ -1,6 +1,7 @@
const schema = [];

const reComponentName = /^(Pure)?Component$/;
const reReadOnly = /^\$(ReadOnly|FlowFixMe)$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

$FlowFixMe was just an example. It can be anything defined in suppress_type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how would we know what's defined there (from within a plugin rule)?

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't. This is why I was suggesting to not throw an error in cases where you don't know it's wrong for sure. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe allow to specify the suppress type name in an option of the rule?


const isReactComponent = (node) => {
if (!node.superClass) {
Expand All @@ -25,23 +26,30 @@ const create = (context) => {
const reportedFunctionalComponents = [];

const isReadOnlyClassProp = (node) => {
return node.superTypeParameters.params[0].id &&
node.superTypeParameters.params[0].id.name !== '$ReadOnly' &&
!readOnlyTypes.includes(node.superTypeParameters.params[0].id.name);
const id = node.superTypeParameters.params[0].id;

return id && !reReadOnly.test(id.name) && !readOnlyTypes.includes(id.name);
};

const isReadOnlyObjectType = (node) => {
return node.type === 'TypeAlias' &&
node.right &&
node.right.type === 'ObjectTypeAnnotation' &&
node.right.properties.length > 0 &&
node.right.properties.every((prop) => {
if (!node || node.type !== 'ObjectTypeAnnotation') {
return false;
}

// we consider `{||}` to be ReadOnly since it's exact AND has no props
if (node.exact && node.properties.length === 0) {
return true;
}

// { +foo: ..., +bar: ..., ... }
return node.properties.length > 0 &&
node.properties.every((prop) => {
return prop.variance && prop.variance.kind === 'plus';
});
};

const isReadOnlyType = (node) => {
return node.type === 'TypeAlias' && node.right.id && node.right.id.name === '$ReadOnly' || isReadOnlyObjectType(node);
return node.type === 'TypeAlias' && node.right.id && reReadOnly.test(node.right.id.name) || isReadOnlyObjectType(node.right);
};

for (const node of context.getSourceCode().ast.body) {
Expand All @@ -65,7 +73,9 @@ const create = (context) => {
message: node.superTypeParameters.params[0].id.name + ' must be $ReadOnly',
node
});
} else if (node.superTypeParameters && node.superTypeParameters.params[0].type === 'ObjectTypeAnnotation') {
} else if (node.superTypeParameters &&
node.superTypeParameters.params[0].type === 'ObjectTypeAnnotation' &&
!isReadOnlyObjectType(node.superTypeParameters.params[0])) {
context.report({
message: node.id.name + ' class props must be $ReadOnly',
node
Expand All @@ -92,7 +102,7 @@ const create = (context) => {
(typeAnnotation = currentNode.params[0].typeAnnotation)) {
if ((identifier = typeAnnotation.typeAnnotation.id) &&
!readOnlyTypes.includes(identifier.name) &&
identifier.name !== '$ReadOnly') {
!reReadOnly.test(identifier.name)) {
if (reportedFunctionalComponents.includes(identifier)) {
return;
}
Expand All @@ -107,7 +117,8 @@ const create = (context) => {
return;
}

if (typeAnnotation.typeAnnotation.type === 'ObjectTypeAnnotation') {
if (typeAnnotation.typeAnnotation.type === 'ObjectTypeAnnotation' &&
!isReadOnlyObjectType(typeAnnotation.typeAnnotation)) {
context.report({
message: currentNode.id.name + ' component props must be $ReadOnly',
node
Expand Down
15 changes: 15 additions & 0 deletions tests/rules/assertions/requireReadonlyReactProps.js
Expand Up @@ -175,6 +175,15 @@ export default {
{
code: 'type Props = {| +foo: string, +bar: number |}; class Foo extends Component<Props> { }'
},
{
code: 'type Props = $FlowFixMe; class Foo extends Component<Props> { }'
},
{
code: 'type Props = {||}; class Foo extends Component<Props> { }'
},
{
code: 'class Foo extends Component<{||}> { }'
},

// functional components
{
Expand All @@ -188,6 +197,12 @@ export default {
},
{
code: 'function Foo() { return <p /> }'
},
{
code: 'function Foo(props: $FlowFixMe) { return <p /> }'
},
{
code: 'function Foo(props: {||}) { return <p /> }'
}
]
};