Reducer in `(S) -> S` format

[hellohuanlin]

I am wondering if you are open to change reducers to (S) -> S format instead of S.() -> S.

1. (S) -> S is more readable and easier to understand than S.() -> S
2. An instance function represents a "belong to" relationship with the instance. A reducer is a function that maps S1 to S2, which i don't think it fits the definition well.
3. Recently I have a careless bug:

   
   // my state here: 
   data class State(
   val extension: Extension
   )
   data class Extension(
   val val = 0
   )

// my reducer here { reduce(payload) // this line updates the extension .copy(extension.copy(val=1)) // this line uses the old copy of extension }

It's very easy to make this kind of mistakes with implicit parameter. 
However, with `(S1) -> S2` format, where I have to explicitly specify which state's extension to use, it's much harder to make such mistakes. 

@gpeal @BenSchwab @elihart 

[elihart]

couldn't this exact same mistake happen with (S1) -> S2, because the state doesn't need to be named so it would have the implicit name of it and you could still accidentally write extension.copy(val=1) instead of it.copy(val=1)

could we lint for .copy inside an execute call instead?

[hellohuanlin]

extension.copy(val=1) would be error for (S1) -> S2

[hellohuanlin]

it's much harder to make mistakes by passing explicit parameter, even if it's it. It's pretty obvious that xxx.copy(it.extension.copy(val=1) refer to the extension of the original state.

[gpeal]

@hellohuanlin I admit that the signature is confusing at first. However, we opted for this syntax because it's so common and it simplifies things a bit. In addition, reducers should be pure and capturing this makes you more likely to write code that way.

[hellohuanlin]

@gpeal can you explain more about why S.() -> S makes it more likely to write pure functions? Thanks!

[gpeal]

@hellohuanlin Because the this is the state in the lambda rather than the captured scope.

[gpeal]

@hellohuanlin It's equally possible to write an impure function though.

The main reason we did this is to optimize for the 90% case of execute { copy(foo = it) } or setState { copy(foo = !foo) }

[hellohuanlin]

Thinking about it more, S.() -> S can be more likely to make mistakes:

val value = 123
execute { 
  copy(
    value = value + 1 // captured value, not state.value
  ) 
} 

On the other hand, with (S) -> S where you have to explicitly specify the state:

val value = 123
execute { state -> 
  state.copy(
    value = state.value + 1 // less likely to make mistakes here. 
  )
}

[elihart]

I don't know, I feel like your example could be turned around the opposite way just as easily

withState { state -> 
  MyRequest.build(state.listingId).execute { 
    state.copy(
      value = state.value + 1 // wrong state is used here
    )
  }
}

I think mistakes can be made both ways, it's hard to say which is more error prone, but I think what we have now works well enough and there wouldn't be a big enough different to justify a change this big

[hellohuanlin]

@elihart I do see what you mean, but I think in this particular case, the developer explicitly use the state from outer closure. I feel this mistake is not as likely to make as other cases, and when it happens, it's more of the developer's responsibility. That being said, I do agree that in this case, S.() -> S wins.

Let's put aside which one is more error prune because it's quite a subjective thing. In principle, the concept of reducer feels more like a static member of the state class, instead of a member of the state instance. That's why I feel it's quite odd and not as readable.

[gpeal]

@hellohuanlin I think we're probably going to wind up going in circles here because it's neither here nor there and valid arguments can be made for either. However, it would take a lot of work not just by Airbnb but every consumer of MvRx to change the API and I have a hard time seeing the justification for that.

@elihart

[gpeal]

@hellohuanlin bump

[hellohuanlin]

@gpeal agree that api change would take a lot of work. closing this issue now.