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

Add require-readonly-react-props rule #400

Merged
merged 11 commits into from
May 15, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .README/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ npm install eslint babel-eslint eslint-plugin-flowtype --save-dev
"comma"
],
"flowtype/require-parameter-type": 2,
"flowtype/require-readonly-react-props": 0,
"flowtype/require-return-type": [
2,
"always",
Expand Down
82 changes: 82 additions & 0 deletions .README/rules/require-readonly-react-props.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
### `require-readonly-react-props`

This rule validates that React props are marked as $ReadOnly. React props are immutable and modifying them could lead to unexpected results. Marking prop shapes as $ReadOnly avoids these issues.

The rule tries its best to work with both class and functional components. For class components, it does a fuzzy check for one of "Component", "PureComponent", "React.Component" and "React.PureComponent". It doesn't actually infer that those identifiers resolve to a proper `React.Component` object.

For example, this will NOT be checked:

```js
import MyReact from 'react';
class Foo extends MyReact.Component { }
```

As a result, you can safely use other classes without getting warnings from this rule:

```js
class MyClass extends MySuperClass { }
```

React's functional components are hard to detect statically. The way it's done here is by searching for JSX within a function. When present, a function is considered a React component:

```js
// this gets checked
type Props = { };
function MyComponent(props: Props) {
return <p />;
}

// this doesn't get checked since no JSX is present in a function
type Options = { };
function SomeHelper(options: Options) {
// ...
}

// this doesn't get checked since no JSX is present directly in a function
function helper() { return <p /> }
function MyComponent(props: Props) {
return helper();
}
```

The rule only works for locally defined props that are marked with a `$ReadOnly` or using covariant notation. It doesn't work with imported props:

```js
// the rule has no way of knowing whether ImportedProps are read-only
import { type ImportedProps } from './somewhere';
class Foo extends React.Component<ImportedProps> { }


// the rule also checks for covariant properties
type Props = {|
+foo: string
|};
class Bar extends React.Component<Props> { }

// this fails because the object is not fully read-only
type Props = {|
+foo: string,
bar: number,
|};
class Bar extends React.Component<Props> { }

// this fails because spreading makes object mutable (as of Flow 0.98)
// https://github.com/gajus/eslint-plugin-flowtype/pull/400#issuecomment-489813899
type Props = {|
+foo: string,
...bar,
|};
class Bar extends React.Component<Props> { }
```


```js
{
"rules": {
"flowtype/require-readonly-react-props": 2
}
}
```


<!-- assertions requireReadOnlyReactProps -->
3 changes: 2 additions & 1 deletion src/configs/recommended.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"flowtype/require-parameter-type": 0,
"flowtype/require-return-type": 0,
"flowtype/require-valid-file-annotation": 0,
"flowtype/require-readonly-react-props": 0,
"flowtype/semi": 0,
"flowtype/space-after-type-colon": [
2,
Expand All @@ -45,4 +46,4 @@
"onlyFilesWithFlowAnnotation": false
}
}
}
}
3 changes: 3 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import objectTypeDelimiter from './rules/objectTypeDelimiter';
import requireCompoundTypeAlias from './rules/requireCompoundTypeAlias';
import requireExactType from './rules/requireExactType';
import requireParameterType from './rules/requireParameterType';
import requireReadonlyReactProps from './rules/requireReadonlyReactProps';
import requireReturnType from './rules/requireReturnType';
import requireTypesAtTop from './rules/requireTypesAtTop';
import requireValidFileAnnotation from './rules/requireValidFileAnnotation';
Expand Down Expand Up @@ -58,6 +59,7 @@ const rules = {
'require-compound-type-alias': requireCompoundTypeAlias,
'require-exact-type': requireExactType,
'require-parameter-type': requireParameterType,
'require-readonly-react-props': requireReadonlyReactProps,
'require-return-type': requireReturnType,
'require-types-at-top': requireTypesAtTop,
'require-valid-file-annotation': requireValidFileAnnotation,
Expand Down Expand Up @@ -104,6 +106,7 @@ export default {
'require-compound-type-alias': 0,
'require-exact-type': 0,
'require-parameter-type': 0,
'require-readonly-react-props': 0,
'require-return-type': 0,
'require-variable-type': 0,
semi: 0,
Expand Down
124 changes: 124 additions & 0 deletions src/rules/requireReadonlyReactProps.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
const schema = [];

const reComponentName = /^(Pure)?Component$/;

const isReactComponent = (node) => {
if (!node.superClass) {
return false;
}

return (

// class Foo extends Component { }
// class Foo extends PureComponent { }
node.superClass.type === 'Identifier' && reComponentName.test(node.superClass.name) ||

// class Foo extends React.Component { }
// class Foo extends React.PureComponent { }
node.superClass.type === 'MemberExpression' &&
(node.superClass.object.name === 'React' && reComponentName.test(node.superClass.property.name))
);
};

const create = (context) => {
const readOnlyTypes = [];
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 isReadOnlyObjectType = (node) => {
return node.type === 'TypeAlias' &&
node.right &&
node.right.type === 'ObjectTypeAnnotation' &&
node.right.properties.length > 0 &&
node.right.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);
};

for (const node of context.getSourceCode().ast.body) {
// type Props = $ReadOnly<{}>
if (isReadOnlyType(node) ||

// export type Props = $ReadOnly<{}>
node.type === 'ExportNamedDeclaration' &&
node.declaration &&
isReadOnlyType(node.declaration)) {
readOnlyTypes.push(node.id ? node.id.name : node.declaration.id.name);
}
}

return {

// class components
ClassDeclaration (node) {
if (isReactComponent(node) && isReadOnlyClassProp(node)) {
context.report({
message: node.superTypeParameters.params[0].id.name + ' must be $ReadOnly',
node
});
} else if (node.superTypeParameters && node.superTypeParameters.params[0].type === 'ObjectTypeAnnotation') {
context.report({
message: node.id.name + ' class props must be $ReadOnly',
node
});
}
},

// functional components
JSXElement (node) {
let currentNode = node;
let identifier;
let typeAnnotation;

while (currentNode && currentNode.type !== 'FunctionDeclaration') {
currentNode = currentNode.parent;
}

// functional components can only have 1 param
if (!currentNode || currentNode.params.length !== 1) {
return;
}

if (currentNode.params[0].type === 'Identifier' &&
(typeAnnotation = currentNode.params[0].typeAnnotation)) {
if ((identifier = typeAnnotation.typeAnnotation.id) &&
!readOnlyTypes.includes(identifier.name) &&
identifier.name !== '$ReadOnly') {
if (reportedFunctionalComponents.includes(identifier)) {
return;
}

context.report({
message: identifier.name + ' must be $ReadOnly',
node
});

reportedFunctionalComponents.push(identifier);

return;
}

if (typeAnnotation.typeAnnotation.type === 'ObjectTypeAnnotation') {
context.report({
message: currentNode.id.name + ' component props must be $ReadOnly',
node
});
}
}
}
};
};

export default {
create,
schema
};
Loading