Skip to content

Commit

Permalink
fix: support union type in React react-only props rule (#477)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mrtnzlml committed Apr 16, 2021
1 parent 0488fcb commit 7cabcda
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 3 deletions.
2 changes: 1 addition & 1 deletion .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.

Expand Down
14 changes: 13 additions & 1 deletion README.md
Expand Up @@ -3451,7 +3451,7 @@ const f: fn = (a, b) => {}
<a name="eslint-plugin-flowtype-rules-require-readonly-react-props"></a>
### <code>require-readonly-react-props</code>
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.
Expand Down Expand Up @@ -3574,6 +3574,13 @@ export type Props = {}; class Foo extends Component<Props> { }
type Props = {| foo: string |}; class Foo extends Component<Props> { }
// Message: Props must be $ReadOnly

type Props = {| foo: string |} | {| bar: number |}; class Foo extends Component<Props> { }
// Message: Props must be $ReadOnly

// Options: [{"useImplicitExactTypes":true}]
type Props = { foo: string } | { bar: number }; class Foo extends Component<Props> { }
// Message: Props must be $ReadOnly

type Props = {| +foo: string, ...bar |}; class Foo extends Component<Props> { }
// Message: Props must be $ReadOnly

Expand Down Expand Up @@ -3622,6 +3629,11 @@ type Props = {| +foo: string |}; class Foo extends Component<Props> { }

type Props = {| +foo: string, +bar: number |}; class Foo extends Component<Props> { }

type Props = {| +foo: string |} | {| +bar: number |}; class Foo extends Component<Props> { }

// Options: [{"useImplicitExactTypes":true}]
type Props = { +foo: string } | { +bar: number }; class Foo extends Component<Props> { }

type Props = $FlowFixMe; class Foo extends Component<Props> { }

type Props = {||}; class Foo extends Component<Props> { }
Expand Down
16 changes: 15 additions & 1 deletion src/rules/requireReadonlyReactProps.js
Expand Up @@ -33,6 +33,7 @@ const isReactComponent = (node) => {
);
};

// type Props = {| +foo: string |}
const isReadOnlyObjectType = (node, {useImplicitExactTypes}) => {
if (!node || node.type !== 'ObjectTypeAnnotation') {
return false;
Expand All @@ -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) => {
Expand Down
32 changes: 32 additions & 0 deletions tests/rules/assertions/requireReadonlyReactProps.js
Expand Up @@ -66,6 +66,27 @@ export default {
},
],
},
{
code: 'type Props = {| foo: string |} | {| bar: number |}; class Foo extends Component<Props> { }',
errors: [
{
message: 'Props must be $ReadOnly',
},
],
},
{
code: 'type Props = { foo: string } | { bar: number }; class Foo extends Component<Props> { }',
errors: [
{
message: 'Props must be $ReadOnly',
},
],
options: [
{
useImplicitExactTypes: true,
},
],
},
{
code: 'type Props = {| +foo: string, ...bar |}; class Foo extends Component<Props> { }',
errors: [
Expand Down Expand Up @@ -164,6 +185,17 @@ export default {
{
code: 'type Props = {| +foo: string, +bar: number |}; class Foo extends Component<Props> { }',
},
{
code: 'type Props = {| +foo: string |} | {| +bar: number |}; class Foo extends Component<Props> { }',
},
{
code: 'type Props = { +foo: string } | { +bar: number }; class Foo extends Component<Props> { }',
options: [
{
useImplicitExactTypes: true,
},
],
},
{
code: 'type Props = $FlowFixMe; class Foo extends Component<Props> { }',
},
Expand Down

0 comments on commit 7cabcda

Please sign in to comment.