Skip to content

Commit

Permalink
feat: add require-readonly-react-props rule (#400)
Browse files Browse the repository at this point in the history
* Add requireReadOnlyReactProps rule

* Update messages

* Change naming

* Fix imports

* Fix linter

* Fix some edge cases

* Fix few more edge cases

* Make covariant notation work

* Update docs
  • Loading branch information
kangax authored and gajus committed May 15, 2019
1 parent 138c288 commit 22dad37
Show file tree
Hide file tree
Showing 7 changed files with 406 additions and 1 deletion.
1 change: 1 addition & 0 deletions .README/README.md
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
@@ -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
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
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
@@ -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
};

0 comments on commit 22dad37

Please sign in to comment.