middleware's getState always returns optional

[dani-mp]

I use ReSwift's middleware a lot, as I consider it one of the killer features of this library, and I've always wondered why the getState function we pass to the middleware returns the state as an optional value, having to write the boilerplate of unwrapping it every time we want to access it, which is super annoying.

I've been thinking about this and, in theory, it doesn't need to be like that, as we always supposed to have a non-optional state once we've instantiated the store, either because we pass a preloaded state to it, or because each reducer creates its own initial value after the init action has been dispatched (which happens at store's init time). In fact, you will never see anyone checking if the state is not null before accessing it in the original Redux library, written in the super dangerous JavaScript.

This past weekend, I've been trying to make the changes needed to make getState non-optional, and after accommodating the code a bit and adding a couple of tests, I've realised this is not possible a priori, so we can avoid a retain cycle that would make the store impossible to deallocate. I've been trying to use the techniques described here, but I couldn't make it work.

What do you think? Should this be possible at all?

[mjarvis]

getState returns an optional in order to allow for the chance that the store gets deallocated during execution of some asynchronous middleware.
While hidden, this also happens with dispatch in middleware -- its just able to be wrapped as a non-optional function because there is no return type (See Store.swift:L74 for the wrapper).

Making getState non-optional is technically possible, but then Middleware writers would be required to understand their role in maintaining a reference to the store during execution. If you have middleware that spawns an infinitely running task, and uses getState, one would end up with zombie object if we try to release the store. The alternative for this would be to pass the store itself into middleware, instead of dispatch and getState, which would then require the middleware itself to deal with weakifying it if they need to.

We can avoid the retain cycle you speak of by not pre-computing the internal dispatch function at init, or by changing it such that it also has a self typed parameter, instead of referencing self directly in the closures.

[dani-mp]

Sorry, I haven't explained myself very well.

I know how to make the getState return type non-optional, something like

let getState: () -> State = { [unowned self] self.state! }

should be enough and, as you already know, if the store gets deallocated and a middleware still uses getState, the app will crash.

I guess my real question is how can we make getState return a non-optional state without letting the app crash if the store has been deallocated, and some other part of the code has still a reference to it (obviously this other part of the code should be responsible of this memory management, but that's always the case when you work with Swift, so I wouldn't think it's a downside of the library itself). Would this make sense?

I think something like this:

The alternative for this would be to pass the store itself into middleware, instead of dispatch and getState, which would then require the middleware itself to deal with weakifying it if they need to.

could be a good idea, but instead of passing it the whole store, just passing it an instance of a lightweight version of the store (with the getState and dispatch methods). Could this work?

This I didn't get it, could you elaborate?

We can avoid the retain cycle you speak of [...] by changing it such that it also has a self typed parameter, instead of referencing self directly in the closures.

[mjarvis]

Its not a straight swap over, but something like changing Middleware to:

public typealias Middleware<State> = (Store<State>) -> (@escaping DispatchFunction) -> DispatchFunction

Then in Store, dispatchFunction becomes public var dispatchFunction: ((Store) -> DispatchFunction)!

and the dispatch function creation becomes

        self.dispatchFunction = { store in
            middleware
            .reversed()
            .reduce(
                { [unowned self] action in
                    self._defaultDispatch(action: action) },
                { dispatchFunction, middleware in
                    return middleware(store)(dispatchFunction)
            })
        }

then finally dispatch will call dispatchFunction(self)(action)

There are many other difficulties in making a switch like this -- We actually want to restrict to StoreType, not Store<State>, and then we run into issues specializing generics in typealias that I haven't looked too far into.

I believe all the issues are workable, though I'm not sure its valuable enough for the effort required.

[dani-mp]

Interesting. Following this approach, maybe instead of passing it the whole store, we can pass it only the tuple with the two functions the current middleware needs? This could simplify the types, I guess, but recalculating the dispatch function for every action dispatched wouldn't be performant (although we could create the function only once, lazily).

I need to play with this to really know that it breaks the retain cycle, as it's still not clear to me, and see if the code could be simplified.

I do think it makes sense getState returns a non-optional value if the store's state has type of State!. It's not only a better API because it'd be less verbose to use, but also the types would be consistent as well, which is nice.

Thanks for the time you're putting into this issue, @mjarvis, you're great.

[a-voronov]

Then in Store, dispatchFunction becomes public var dispatchFunction: ((Store) -> DispatchFunction)!

and the dispatch function creation becomes

       self.dispatchFunction = { store in
           middleware
           .reversed()
           .reduce(
               { [unowned self] action in
                   self._defaultDispatch(action: action) },
               { dispatchFunction, middleware in
                   return middleware(store)(dispatchFunction)
           })
       }

If I understood you correctly, this approach might break some middlewares' current behavior.
Judging by its type (DispatchFunction, () -> State?) -> (DispatchFunction) -> DispatchFunction, there're multiple steps where middleware is called (but actually major two):

• (DispatchFunction, () -> State?) -> DispatchFunction -> ...
• ... -> DispatchFunction

let middleware: Middleware<State> = { dispatch, getState in
    // both, this 
    return { next in
        // and this closures are called only once on Store init
        // thus you can store some local state here

        return { action in
            // and this closure is called for every action
        }
    }
}

But if you put whole middleware appliance into dispatchFunction then all three closures will be called for every action every single time.

[mjarvis]

@a-voronov Ah! Thats a very interesting point! a second downside to that is the two extra closure calls per middleware per action could be a not-insignificant performance hit, as well.