fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
275 stars 67 forks source link

Consider marking the setState(state) overload with a warning or removing it #104

Closed SirUppyPancakes closed 5 years ago

SirUppyPancakes commented 5 years ago

In react, when setting the state you can do: this.setState({ a: 123 }) or this.setState((state, props) => { a: state + 1 }) (if you need the current state to calculate the next state)

Doing this.setState({ a: this.state + 1 }) is documented (but not necessarily well-known) to be incorrect and a source of bugs (since setState calls are asynchronous and batched, is its possible for this.state to be stale; see https://reactjs.org/docs/state-and-lifecycle.html#state-updates-may-be-asynchronous for more information on this).

In JavaScript, the first overload is still useful though, because you don't always need the original state, and you can specify only the properties you wish to change (and it will merge them in for you): this.setState({ a: 123 }) when props is { a: number, b: number } will keep b and merge in the new value for a.

In Fable however, this is not even possible (when staying within 100% type-safe code). In order to set the state, you have to pass a full instance of the state type specified when creating the component (Component<'P, 'S>) which results in code like this: this.setState({ this.state with a = 123 }) which looks correct at first, but is a source of bugs since it uses this.state.

The only way around this--until F# gets a way to specify sparse, partial types derived from other types (which languages like TypeScript have via mapped types)--is to always use the callback version of setState: this.setState(fun state props -> { state with a = 123 })

Since there isn't a correct usage of the first overload of this.setState due to F# immutability, I think we should mark it with some sort of warning or remove it altogether.

Additionally, the second overload this.setState(updater) actually also takes an optional callback this.setState(updater[, callback]) which is a simple unit -> unit function that is called when this.state is done being updated and ready to use.

alfonsogarciacaro commented 5 years ago

Thanks a lot for the explanation @SirUppyPancakes. I think that makes sense, if you want you can send a PR to add the Obsolete attribute to the setState(s: 'S) with a message saying the overload is not safe so devs get a warning when using it.