Flow: React component's props and state should be Exact and Readonly

[rickyp-uber]

Props and State should always be Exact and ReadOnly, preferably recursively. No component should have the ability to edit the props nor state directly, instead props are defined by the parent and the state should only be modified via setState (based on React documentation)

https://flow.org/try/#0FASwtgDg9gTgLgAgFQIIYGcECUCmqDGiAZjFGAgOQx6EUDcwwOAHtPAnAJ4Q4ICq6HDAAq4HABsQAOxwAFUhEwBeBAG9gCBBAUBGAFwJ0cGNIDmwAL4MmrWIi49+gkWMkyAynFRxeK9ZqNvHH1DYzNLaxY2RHxxDEwBIVEwCWleFh8pABNMXAI4ADoAYTJoGSk4AB4NJyTXNPkoRQAaGsSXFLccTyDgAD41GvwoKSMYAFdCWAAKbSb0A3bk1JlGxQBKQc0A8Z4YWYV0dYZtjgALEHQCwJ8EPxrTm+CDACILl4eEKxqLGpr0M5QcbiLIlSAjHAVPgQLJBabrAwAIygUHEeCkW22AHosQhhBdMACgSCEEJSDBDNJ8LwpFAEBDDIDgVlSQA3IQILJ0uAEgp8tlCGoASB5l2uXh84qCOjuCDeEnEUBeJwQwCF1Dg4xgGKIqHEghVvwsQA

[wchargin]

Agreed. Unfortunately, not only is making State read-only not
required, it’s not possible:

import * as React from 'react';

export type UserTimelineState = {
  +field: string
};

export class UserTimeline extends React.Component<{}, UserTimelineState> {
  constructor(props: {}) {
    super(props);
    this.state = {field: "initial"};
  }
  someCallback() {
    this.setState({field: "hello"});
  }
}

(flow.org/try)

This yields an error on the setState call:

Cannot call this.setState with object literal bound to
partialState because property field is not writable in property
field.

It would be lovely if this could be changed to respect the semantics
that you describe.

[mrkev]

My thoughts:

| | $Exact | $ReadOnly |
|-------|------------------|--------|
| state | Ok | Yes |
| props | Unsure [1] | Yes |

class Component extends React.Component<{a: 1}> { }

function f () {
    const foo = {a: 1, b: 2}
    return <Component ...foo>
}

Idk enough about this to know if it matters. If it does, and the above use is bad practice, then I'm all for it. Otherwise, we'd just be making devs lives harder for nothing really ¯\_(ツ)_/¯

[rickyp-uber]

I think that makes sense, but at least making them readonly I think makes semantic sense considering how the react lifecycle is supposed to work.

• React's documentation about props being read-only - https://reactjs.org/docs/components-and-props.html#props-are-read-only
  
  
  
  • React's documentation about limiting affecting state directly - https://reactjs.org/docs/state-and-lifecycle.html#do-not-modify-state-directly
  
  

As for $Exact props, I think it depends on the React documentation and passing unknown props to a react component. I know that react will yell at you if you send "font-family" down as a prop in an svg element vs "fontFamily" which is where I thought exact would be useful here; to turn the failure from a run-time error to a compile-time error. The only way that would be possible is through the use of $Exact

[rickyp-uber]

As for @wchargin's comment, I wasn't thinking about making the overall type of React$Component.state read only, more just the internal variable here:
https://github.com/facebook/flow/blob/master/lib/react.js#L30

This should make the property this.state read-only but the rest of the usage (e.g. setState)

[vicapow]

Is this something the FB team would welcome if we submitted a PR?

[vicapow]

Hello again,

Any chance there's some internal discussion happening on this thread we could have visibility into? Any feedback here would be appreciated.

All the best,
Victor

[vicapow]

cc: @avikchaudhuri

[vicapow]

Hello,

Wanted to follow up again. With guidance, we'd be happy to help implement this.

Best,
Victor

[lxe]

Hardcoding a specific usage patterns of a specific library into a tool/language is ill-advised. Shouldn't this live in React's specific libdef? Or even with your own component:

https://flow.org/try/#0FASwtgDg9gTgLgAgFQIIYGcECUCmqDGiAZjFGAgOQx6EUDcwwOAHtPAnAJ4Q4ICq6HDAAq4HABsQAOxwAFUhEwBeBAG9gCBBAUBGAFwJ0cGNIDmwAL4MmrWIi49+gkWMkyAynFRxeKgCS4qAAmAPJS4pwAPOqaRt44+obGZpYAfNYsbIj44hiYAkKiYBLSvCw+UkGYgYQAdADCZNAyUnCRGk6FrqXyUIoANB0FLsVuOJ7xwKlqHfhQUkYwAK6EsAAU2n3oBsNFJTK9igCUM5qxSzwwGwroRwxnHAAWIOi1cT4IKjEPSfGJAETPf4dTRWDoWDoddCPKBLcRBRqQeY4Vp8CBBeJrI4GABGUCg4jwUlOZwA9KSEMJnphobD4QghKQYIZpPheFIoAhkYYYXCggyAG5CBBBTlwam1SWCoQdACQ4peby8PiVf0+CEBEnEUH+9wQwFl1DgSxgxKIqHEgj1EIsQA

I don't think there isn't anything in React's implementation that guarantees that assigning directly to state will always cause undefined behavior. In fact, state behavior and how to use it is well-documented. There are probably even reasons to mutate the state object sometimes.

[wchargin]

@lxe: It should live in React's libdef, which is part of the Flow repository in lib/react.js. (You may validly object to this, but that's how it is.) Thus, this is a change to Flow.

That you should not mutate state is clearly documented:

Do Not Modify State Directly

[…]

// Wrong
this.state.comment = 'Hello';

Thus, this is in fact part of the React API.

[kangax]

Would love to have this enforced by Flow.

In the meantime, I've transitioned our codebase to always use $ReadOnly for Props/State (we already enforce exact objects app-wide) and will try to add a lint rule for it this week — https://github.com/gajus/eslint-plugin-flowtype/issues/397