From 7cabcda48a940fcb8f30b32e30481fe636ccb838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Zl=C3=A1mal?= Date: Fri, 16 Apr 2021 20:27:38 +0200 Subject: [PATCH] fix: support union type in React react-only props rule (#477) This change fixes the following (simplified) example from our application: ``` type Props = | { +children: React$Node, +title: React$Node, +withHiddenTitle?: false, } | { +children: React$Node, +title?: React$Node, +withHiddenTitle: true, }; const props: Props = {children:"", withHiddenTitle: true} props.title = ""; // correct Flow error ``` Flow correctly throws the following error: ``` 14: props.title = ""; ^ Cannot assign empty string to `props.title` because property `title` is not writable. [cannot-write] ``` However, `flowtype/require-readonly-react-props` Eslint rule throws the following (incorrect) error: ``` Props must be $ReadOnly ``` For simplification, all of the objects in the union must be readonly even though it's probably not a strict requirement in Flow. This can be improved later when needed. --- .README/rules/require-readonly-react-props.md | 2 +- README.md | 14 +++++++- src/rules/requireReadonlyReactProps.js | 16 +++++++++- .../assertions/requireReadonlyReactProps.js | 32 +++++++++++++++++++ 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/.README/rules/require-readonly-react-props.md b/.README/rules/require-readonly-react-props.md index 2ccf54f..ab414ad 100644 --- a/.README/rules/require-readonly-react-props.md +++ b/.README/rules/require-readonly-react-props.md @@ -1,6 +1,6 @@ ### `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. +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. diff --git a/README.md b/README.md index aca2fd1..7ad3d6f 100644 --- a/README.md +++ b/README.md @@ -3451,7 +3451,7 @@ const f: fn = (a, b) => {} ### 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. +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. @@ -3574,6 +3574,13 @@ export type Props = {}; class Foo extends Component { } type Props = {| foo: string |}; class Foo extends Component { } // Message: Props must be $ReadOnly +type Props = {| foo: string |} | {| bar: number |}; class Foo extends Component { } +// Message: Props must be $ReadOnly + +// Options: [{"useImplicitExactTypes":true}] +type Props = { foo: string } | { bar: number }; class Foo extends Component { } +// Message: Props must be $ReadOnly + type Props = {| +foo: string, ...bar |}; class Foo extends Component { } // Message: Props must be $ReadOnly @@ -3622,6 +3629,11 @@ type Props = {| +foo: string |}; class Foo extends Component { } type Props = {| +foo: string, +bar: number |}; class Foo extends Component { } +type Props = {| +foo: string |} | {| +bar: number |}; class Foo extends Component { } + +// Options: [{"useImplicitExactTypes":true}] +type Props = { +foo: string } | { +bar: number }; class Foo extends Component { } + type Props = $FlowFixMe; class Foo extends Component { } type Props = {||}; class Foo extends Component { } diff --git a/src/rules/requireReadonlyReactProps.js b/src/rules/requireReadonlyReactProps.js index 41ffee6..b1e553c 100644 --- a/src/rules/requireReadonlyReactProps.js +++ b/src/rules/requireReadonlyReactProps.js @@ -33,6 +33,7 @@ const isReactComponent = (node) => { ); }; +// type Props = {| +foo: string |} const isReadOnlyObjectType = (node, {useImplicitExactTypes}) => { if (!node || node.type !== 'ObjectTypeAnnotation') { return false; @@ -55,8 +56,21 @@ const isReadOnlyObjectType = (node, {useImplicitExactTypes}) => { }); }; +// type Props = {| +foo: string |} | {| +bar: number |} +const isReadOnlyObjectUnionType = (node, options) => { + if (!node || node.type !== 'UnionTypeAnnotation') { + return false; + } + + return node.types.every((type) => { + return isReadOnlyObjectType(type, options); + }); +}; + const isReadOnlyType = (node, options) => { - return node.right.id && reReadOnly.test(node.right.id.name) || isReadOnlyObjectType(node.right, options); + return node.right.id && reReadOnly.test(node.right.id.name) || + isReadOnlyObjectType(node.right, options) || + isReadOnlyObjectUnionType(node.right, options); }; const create = (context) => { diff --git a/tests/rules/assertions/requireReadonlyReactProps.js b/tests/rules/assertions/requireReadonlyReactProps.js index 3332013..67af5f7 100644 --- a/tests/rules/assertions/requireReadonlyReactProps.js +++ b/tests/rules/assertions/requireReadonlyReactProps.js @@ -66,6 +66,27 @@ export default { }, ], }, + { + code: 'type Props = {| foo: string |} | {| bar: number |}; 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', + }, + ], + options: [ + { + useImplicitExactTypes: true, + }, + ], + }, { code: 'type Props = {| +foo: string, ...bar |}; class Foo extends Component { }', errors: [ @@ -164,6 +185,17 @@ export default { { code: 'type Props = {| +foo: string, +bar: number |}; class Foo extends Component { }', }, + { + code: 'type Props = {| +foo: string |} | {| +bar: number |}; class Foo extends Component { }', + }, + { + code: 'type Props = { +foo: string } | { +bar: number }; class Foo extends Component { }', + options: [ + { + useImplicitExactTypes: true, + }, + ], + }, { code: 'type Props = $FlowFixMe; class Foo extends Component { }', },