facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.07k stars 1.85k forks source link

[Proposal] Mark state & prop arrays in components & hooks as readonly #9152

Closed Ayc0 closed 2 months ago

Ayc0 commented 4 months ago

Proposal

When using the an object as a prop or as a state, Flow prevents you from mutating it directly (like state.attr = value or with state[attr] = value) But when using arrays, you can still use state.push() or state.sort() and other mutating methods (but not state[0] = value as this is already covered by the other rule)

Proposal: automatically replace Array for $ReadOnlyArray for states & props

REPL

image

Use case

Make sure that the rules of React are getting applied for even more edge cases: props & states should never be mutated manually (as it can break re-renders). Flow already does some internal computation to detect some mutation, but ReadOnlyArray already exists for this exact use-case for arrays

SamChou19815 commented 4 months ago

This is an intentional choice. We have tried to automatically transform it into $ReadOnlyArray, but it's just too impractical to rollout on a large codebase.

Ayc0 commented 4 months ago

What about with a settings under [options] in the .flowconfig file?

SamChou19815 commented 4 months ago

That can be a good idea, but we do not have time to implement this now. We can accept contributions. Note that it will still not catch all problems, since we also completely give up on class methods, and assume whatever you call don't mutate things.

Ayc0 commented 3 months ago

I thought more about this: there is the same issue for all objects passed in props, and also deep objects. So if there is an object with an array instead, both should be marked as readonly

mvitousek commented 2 months ago

In the next Flow release, Flow will now ban mutating methods from being called on Arrays, Sets, and Maps. We still don't treat them entirely as ReadOnlyArrays for the purposes of passing them to other functions, which may cause mutation, because the rollout would be impractical, but the example here will now error as expected.