updateState & withState with state inside receiver potentially leads to overlooking the problems

[y9vad9]

Problem

FlowMVI provides useful functions to prevent state racing through updateState and withState functions, but sometimes the way it's declared can lead to potential problem overlooking. The problem is that the state is handled in the receiver of the lambda, not as a parameter.

For example:

reduce { intent ->
    when (intent) {
        is EmailChange -> updateState { copy(email = EmailAddress(intent.value)) }
        is ButtonClicked -> updateState {
            copy(email = email.validate()).let { 
                if (email.isValid) { // we used previous state that isn't validated, what means it'll always be false
                   it.copy(isLoading = true)
                   authorizeAsync(..)
                } else it
            }
        }
    }
}

Possible solution

To avoid such problems, but within parameters in the chain of inner lambdas, Intellij Idea has dedicated inspection that warns when you have the same parameters names. For the receivers, there's no such validation or check (except of DSLs contexts) and it makes updateState and withState potentially problematic even in simple cases.

---

Introducing the separate functions without such problem can help, but does not solve problem fully as we can't be sure that someone else will use the right function. In addition, it adds a new layer of complexity, as there's already useState, withState and updateState that you should understand.

What would I do?

• Deprecate actual function with, at least, DeprecationLevel.WARNING level or with RequiresOptIn (it will make able people to opt-in the error / warning on the project level and will not break existing code).
• Introduce new functions that will provide state as a parameter, but not as receiver.

Overall, I think everything except of DSLs builders that requires particular contexts to run, should be handled using parameters as it's more obvious in case I described and, for example, when looking on code on platforms like GitHub, but not in IDE.

[Nek-12]

Thanks for the suggestions and a detailed explanation. So my opinion on this discussion for now is:

1. I agree that having the receiver can indeed be problematic and pollute the scope. Because of the difficulties of mutating state when using it as a parameter, it was decided to use the receiver. The main reason that there are other cases where an it parameter is already used as well (reduce, whileSubscribed, etc).
2. We unfortunately can't deprecate the updateState and withState functions as they are used on practically every second line of code of any store. Replacing them would be a huge effort for any project involved and is only viable for a major version bump (e.g. 2.x -> 3.0). Besides, the names of the deprecated functions are colliding with the new function names (how to name them if these two are already taken?)

Workarounds for now:

1. The simplest solution our team uses right now is to embrace the scope overrides. Your example is solved with:

is ButtonClicked -> updateState {
    copy(email = email.validate()).run { // notice we replaced the scope 
        if (!email.isValid) return@updateState this
        authorizeAsync(..)
        copy(isLoading = true)
    }
}

This code looks simpler, behaves in the same way, has less nesting and has no problems of the previous code. The downside is that it's harder to understand outside of the IDE.

My resolution ideas:

1. I agree that useState is the most confusing function of them all. A better name for useState would be something like updateStateImmediate which shows how it will behave more clearly. In 2.5, you can expect the useState function to be deprecated with an auto-rename.
2. We don't have to settle with not having lint checks against that. I have a couple of lint checks in mind for an upcoming IDE plugin (maybe even a detekt plugin?). Creating a lint check for this issue should be fairly simple. I can't promise this for 2.5, but I've got this in my backlog.