Add require-readonly-react-props rule

[kangax]

Closes #397

require-readonly-react-props

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

This rule validates that React props are marked as $ReadOnly. React props are immutable and modifying them could lead to unexpected results (I've seen people try to do this in our codebase, not realising it doesn't work). 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:

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

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

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:

// 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. It doesn't work with imported props or props marked as read-only via covariant notation:

// 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 doesn't check for covariant properties, even though technically this object is entirely read-only
type Props = {|
    +foo: string
|};
class Bar extends React.Component<Props> { }

[kangax]

I'm gonna test this PR against our codebase (~180K LOC with ~65% of it flowified) and report back if there are any issues

[kangax]

@gajus I've fixed few more edge cases and this now runs successfully in our codebase. Would love to have this tested by someone else. Let me know if there's anything else to do here before this could be merged.

[gajus]

It doesn't work with [..] props marked as read-only via covariant notation:

Is there a reason for this?

I almost always use {|+|} notation over $ReadOnly. Not familiar if there is a difference or if one should be preferred over the other.

[kangax]

@gajus afaik, there's no difference. I didn't want to go down that route as it seemed too complex but now that I think about it... I can just check all top-level object attributes and determine easily if they're all "read-only". I'll update the PR with the fix and tests.

Here's something else I just found. Apparently, spreading invalidates immutability of the entire shape 🤦🏻‍♂️

https://flow.org/try/#0C4TwDgpgBA8sAWEBOAVc0C8UDeAfKAUFFANQBGAhgF4BcUAzsEgJYB2A5gbgL4DcBBUJCgAxAPZioWPEVKUkdRiw4AaWQDpNcRKnRqe-AgGMxrRlABmEuuMnSo8ugHIAHk5UPqzkE6h8BAPQBUKySyEhiSPRQEABuEKxQCGIAruzwUABE8plQFKwAJlmUVLn08KkANkVkYggO0EgQFAUAtKaVIARWYuryUlCubvw9fdQDTiA+hgRBMUgRUVCpwFAmCxBGwJ2C6KISAEwDACQASs0FMKydADwyxJ4KDExs7GoPmurayGiQ+twAPkMJjMqx6BxshwG2EezjcHhK3l8-m6hzGSAmLmGqLEBzGVAmUycvCAA

I guess we can bail out if we encounter spreading, so:

type Foo = {| +foo: string |} // read-only
type Bar = $ReadOnly<{| foo: string |}> // read-only

type Baz = {| +foo: string, ...Bar |} // NOT read-only

[gajus]

TIL that this is not an error:

type Bar = {| +bar: string |}
type Baz = {| +foo: string, ...Bar |}

let baz: Baz = {foo: 'foo', bar: 'baz'};

baz.foo = 'test';
baz.bar = 'test';

My guess that it is related to facebook/flow#7298.

[kangax]

@gajus made covariant notation work

[kangax]

@gajus any chance to get this merged? 🙏

[gajus]

🎉 This PR is included in version 3.9.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[mrtnzlml]

Hello @kangax! 👋 I tried this rule in our monorepo and I found a few cases where this rule fails but it probably should not:

1. component with Flow suppress_type instead of props:

export function MonoText(props: $FlowFixMe) {
  return <Text ... />;
}

  8:5  error  $FlowFixMe must be $ReadOnly  flowtype/require-readonly-react-props

2. Relay props:

import type { AppQueryResponse } from './__generated__/AppQuery.graphql';

// note that AppQueryResponse is actually exact and readonly
function renderQueryRendererResponse(props: AppQueryResponse) {
  return <div ... />
}

  31:5  error  AppQueryResponse must be $ReadOnly  flowtype/require-readonly-react-props

3. complicated props:

type CommonProps = {|
  +query: GraphQLTaggedNode,
  +clientID?: string, // Identification of the current client (X-Client header basically).
  +environment?: Environment,
  +cacheConfig?: {|
    +force?: ?boolean,
    +poll?: ?number,
  |},
  +variables?: Variables,
|};

// ...

type Props =
  | {|
      ...CommonProps,
      +onSystemError?: ({ error: Error, retry: ?() => void }) => React$Node,
      +onLoading?: () => React$Node,
      +onResponse: RendererProps => React$Node,
    |}
  | {|
      ...CommonProps,
      +render: ReadyState => React$Node,
    |};

export default function QueryRenderer(props: Props) {
  // ...
}

  # this error line was not correct (?)
  118:5  error  Props must be $ReadOnly  flowtype/require-readonly-react-props

4. empty props (not accepting props at all - exact):

export default class HomeScreen extends React.Component<{||}> {
  // ...
}

  17:16  error  HomeScreen class props must be $ReadOnly  flowtype/require-readonly-react-props

I hope this will help you to make some additional adjustments and improvements. Cheers 🍻

[kangax]

Hey @mrtnzlml, thanks for these.

component with Flow suppress_type instead of props:

I'll make a PR shortly to fix this.

Relay props:

we have no way of knowing if imported props are read-only, this is documented in the rule description

complicated props:

I'd love to see more examples here because in this case you're spreading an object, which makes a "parent" shape mutable (likely a Flow bug)

empty props (not accepting props at all - exact):

Hm, I guess we should fix this only if an empty object is both exact and empty (and is essentially immutable)

[mrtnzlml]

we have no way of knowing if imported props are read-only, this is documented in the rule description

I hope you'll reconsider the error. Clearly, it's false positive and it essentially means we'll never enable this rule because it's very common. That would be a pity. I think a better strategy would be to be silent if you are not sure instead of throwing this:

  31:5  error  AppQueryResponse must be $ReadOnly  flowtype/require-readonly-react-props

Hm, I guess we should fix this only if an empty object is both exact and empty (and is essentially immutable)

Yes please, this is exactly how we are using it: to say "this component doesn't accept props".

[ngyikp]

Having a React component with no props/state defined causes a crash:

import { Component } from 'react';

class TestComponent extends Component {
  render() {
    return <div>Hello</div>;
  }
}

Error:

TypeError: Cannot read property 'params' of undefined
Occurred while linting main.js:3
    at isReadOnlyClassProp (node_modules/eslint-plugin-flowtype/dist/rules/requireReadonlyReactProps.js:32:37)
    at ClassDeclaration (node_modules/eslint-plugin-flowtype/dist/rules/requireReadonlyReactProps.js:80:37)
    at listeners.(anonymous function).forEach.listener (node_modules/eslint/lib/util/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (node_modules/eslint/lib/util/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (node_modules/eslint/lib/util/node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (node_modules/eslint/lib/util/node-event-generator.js:280:22)
    at NodeEventGenerator.enterNode (node_modules/eslint/lib/util/node-event-generator.js:294:14)
    at CodePathAnalyzer.enterNode (node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:632:23)
    at nodeQueue.forEach.traversalInfo (node_modules/eslint/lib/linter.js:752:32)

ESLint config:

{
	"rules": {
		"flowtype/require-readonly-react-props": "error"
	},
	"parser": "babel-eslint",
	"plugins": ["flowtype"]
}