diff --git a/.README/README.md b/.README/README.md index 701dd03a..b8a25873 100644 --- a/.README/README.md +++ b/.README/README.md @@ -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", diff --git a/.README/rules/require-readonly-react-props.md b/.README/rules/require-readonly-react-props.md new file mode 100644 index 00000000..5ac1d62e --- /dev/null +++ b/.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

; +} + +// 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

} +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 { } + + +// the rule also checks for covariant properties +type Props = {| + +foo: string +|}; +class Bar extends React.Component { } + +// this fails because the object is not fully read-only +type Props = {| + +foo: string, + bar: number, +|}; +class Bar extends React.Component { } + +// 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 { } +``` + + +```js +{ + "rules": { + "flowtype/require-readonly-react-props": 2 + } +} +``` + + + \ No newline at end of file diff --git a/src/configs/recommended.json b/src/configs/recommended.json index 96fe620a..5f7cdabc 100644 --- a/src/configs/recommended.json +++ b/src/configs/recommended.json @@ -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, @@ -45,4 +46,4 @@ "onlyFilesWithFlowAnnotation": false } } -} +} \ No newline at end of file diff --git a/src/index.js b/src/index.js index b791193f..aed03f9f 100644 --- a/src/index.js +++ b/src/index.js @@ -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'; @@ -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, @@ -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, diff --git a/src/rules/requireReadonlyReactProps.js b/src/rules/requireReadonlyReactProps.js new file mode 100644 index 00000000..c40bdfd0 --- /dev/null +++ b/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 +}; diff --git a/tests/rules/assertions/requireReadonlyReactProps.js b/tests/rules/assertions/requireReadonlyReactProps.js new file mode 100644 index 00000000..9cd62284 --- /dev/null +++ b/tests/rules/assertions/requireReadonlyReactProps.js @@ -0,0 +1,193 @@ +export default { + invalid: [ + + // class components + { + code: 'type Props = { }; class Foo extends React.Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type OtherProps = { foo: string }; class Foo extends React.Component { }', + errors: [ + { + message: 'OtherProps must be $ReadOnly' + } + ] + }, + { + code: 'class Foo extends React.Component<{}> { }', + errors: [ + { + message: 'Foo class props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = { bar: {} }; class Foo extends React.Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = { }; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = { }; class Foo extends PureComponent { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'class Foo extends React.Component { }', + errors: [ + { + message: 'UnknownProps must be $ReadOnly' + } + ] + }, + { + code: 'export type Props = {}; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = {| foo: string |}; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = {| +foo: string, ...bar |}; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = {| +foo: string, -bar: number |}; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + + // functional components + { + code: 'type Props = { }; function Foo(props: Props) { return

}', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = { }; function Foo(props: Props) { return foo ?

: }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'function Foo(props: {}) { return

}', + errors: [ + { + message: 'Foo component props must be $ReadOnly' + } + ] + }, + { + code: 'function Foo(props: UnknownProps) { return

}', + errors: [ + { + message: 'UnknownProps must be $ReadOnly' + } + ] + }, + { + code: 'export type Props = {}; function Foo(props: Props) { return

}', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + } + ], + valid: [ + + // class components + { + code: 'class Foo extends React.Component<$ReadOnly<{}>> { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends React.Component { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends React.PureComponent { }' + }, + { + code: 'class Foo extends React.Component<$ReadOnly<{}, State>> { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends React.Component { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends Component { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends PureComponent { }' + }, + { + code: 'type FooType = {}; class Foo extends Bar { }' + }, + { + code: 'class Foo { }' + }, + { + code: 'export type Props = $ReadOnly<{}>; class Foo extends Component { }' + }, + { + code: 'export type Props = $ReadOnly<{}>; export class Foo extends Component { }' + }, + { + code: 'type Props = {| +foo: string |}; class Foo extends Component { }' + }, + { + code: 'type Props = {| +foo: string, +bar: number |}; class Foo extends Component { }' + }, + + // functional components + { + code: 'type Props = {}; function Foo() { }' + }, + { + code: 'type Props = $ReadOnly<{}>; function Foo(props: Props) { }' + }, + { + code: 'type Props = {}; function Foo(props: OtherProps) { }' + }, + { + code: 'function Foo() { return

}' + } + ] +}; diff --git a/tests/rules/index.js b/tests/rules/index.js index 63b5defd..d9458f12 100644 --- a/tests/rules/index.js +++ b/tests/rules/index.js @@ -31,6 +31,7 @@ const reportingRules = [ 'require-compound-type-alias', 'require-exact-type', 'require-parameter-type', + 'require-readonly-react-props', 'require-return-type', 'require-types-at-top', 'require-valid-file-annotation',