1929079 - Investigate usages of `MiddlewareContext::dispatch`

Status: RESOLVED WONTFIX

(Firefox for Android :: General, task, P3)

Description

[Matt Tighe [:matt-tighe]]

In the lib-state implementation of Stores, there are two dispatch functions: Store::dispatch and MiddlewareContext::dispatch. The middleware version interrupts the current action queue to place an action at the beginning. This allows the introduction of quite a bit of complexity in the action processing model, and has the potential to be confusing to anyone not aware of the difference. These differences are only made clear through kdocs and reading the implementation details.

Investigate whether we still need the middleware dispatch method. It is used quite a bit in BrowserStore, but can these usages be safely converted to the Store::dispatch instead?

Comment 1

[Petru-Mugurel Lingurar [:petru]]

I think the middleware needs it's own dispatch because inside of middleware instances we don't have access to the store do call Store::dispatch.

 

The middleware version interrupts the current action queue to place an action at the beginning.

This is an important feature of our middlewares that is nicely documented here.

A [Middleware] sits between the store and the reducer. It provides an extension point between
dispatching an action, and the moment it reaches the reducer.

A [Middleware] can rewrite an [Action], it can intercept an [Action], dispatch additional
[Action]s or perform side-effects when an [Action] gets dispatched.

The [Store] will create a chain of [Middleware] instances and invoke them in order. Every
[Middleware] can decide to continue the chain (by calling next), intercept the chain (by not
invoking next). A [Middleware] has no knowledge of what comes before or after it in the chain.

So the middleware version can interrupt the current action queue to place an action at the beginning if we first do MiddlewareContext::dispatch and then call next(action).
But if inside the invoke method of the middleware we call next(action) and then MiddlewareContext::dispatch the initial action will be processed by all reducers and middlewares and then the new action will go through this chain.

In my opinion we should not modify anything about this functionality.

Comment 2

[Matt Tighe [:matt-tighe]]

Where is interruption documented, specifically? I see in that comment that a middleware can intercept or rewrite an Action, both of which can be accomplished by calling (or not calling) next and don't require a separate dispatch.

I will also note that our implementation of having a separate middleware dispatch is not behavior that matches the JS Redux implementation, for whatever the prior art is worth in this case:

[applyMiddleware] only exposes a subset of the store API to the middleware: dispatch(action) and getState().

The underlying behavior difference in our middleware is documented in this snippet from the middleware dispatch comment:

     * This method is particular useful if a middleware wants to dispatch an additional [Action] and
     * wait until the [state] has been updated to further process it.

I am questioning whether we want two fundamentally different flows for the ways Actions pass through these components.

1. Actions are processed by middleware as they are received by the Store.
   vs
2. Middleware have full control over the Action processing pipeline and can even hijack the Store processing by dispatching Actions repeatedly.

It could be that (2) is useful and necessary for our existing infrastructure, but I am hoping that a deeper investigation would allow us to streamline this area with no loss of functionality.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

(In reply to Petru-Mugurel Lingurar [:petru] from comment #1)

I think the middleware needs it's own dispatch because inside of middleware instances we don't have access to the store do call Store::dispatch.

I was wrong with the above.
The middleware exposes the store to which it is applied, it's just that if we call Store::dispatch the new action will be added to the queue
While if we call MiddlewareContext::dispatch the new action will be put at the front of a new middlewares/reducer chain - effectively "interrupting" the evaluation of the initial action.

I feel though that this is an important functionality, in line with Redux if we look at the "Redux middleware are written as a series of three nested functions" section where we see

return function handleAction(action) {
      // Do anything here: pass the action onwards with next(action),
      // or restart the pipeline with storeAPI.dispatch(action)

and

If you call this dispatch function, it will send the action to the start of the middleware pipeline.

Which seem to describe exactly what our MiddlewareContext::dispatch does

Comment 4

[Petru-Mugurel Lingurar [:petru]]

(In reply to Matt Tighe [:matt-tighe] from comment #2)

Where is interruption documented, specifically? I see in that comment that a middleware can intercept or rewrite an Action, both of which can be accomplished by calling (or not calling) next and don't require a separate dispatch.

 
Yes, for me the documentation for a middleware being able to "interrupt" evaluating the current action in the current chain of middlewares / reducer is based on the documentation of it's dispatch method:

Dispatches an [Action] synchronously on the [Store]. Other than calling [Store.dispatch], this
will block and return after all [Store] observers have been notified about the state change.
The dispatched [Action] will go through the whole chain of middleware again.

This method is particular useful if a middleware wants to dispatch an additional [Action] and
wait until the [state] has been updated to further process it.

Seems like a very nice feature and I don't see any reason to remove it.

 
Middlewares (ours at least) indeed can do anything, as their documentation says:

A Middleware can rewrite an Action, it can intercept an Action, dispatch additional Actions or perform side-effects when an Action gets dispatched.

but I think this is in line with the Redux middlewares - documentation linked above which says

function handleAction(action) {
// Do anything here
}

Comment 5

[Matt Tighe [:matt-tighe]]

If you call this dispatch function, it will send the action to the start of the middleware pipeline.

Which seem to describe exactly what our MiddlewareContext::dispatch does

MiddlewareContext::dispatch will send it to the reducer chain that is constructed in ReducerChainBuilder. This is not the same start of the pipeline as calling Store::dispatch. I also think that in our case that the "start of the pipeline" does not need to imply "skips the line waiting at the start of the pipeline". I am only hoping to see if we can simplifying this dinction. I also don't think it was the intent of the JS Redux docs to imply the two would be treated separately, based on other parts of the same statement you referenced:

These are the same dispatch and getState functions that are actually part of the store. If you call this dispatch function, it will send the action to the start of the middleware pipeline.

Specifically, MiddlewareContext::dispatch is not the same dispatch function that is part of the store in our case. That said, I am not a JS Redux expert. Perhaps :boek can weigh in here as I believe he has more experience with it.

Seems like a very nice feature and I don't see any reason to remove it.

What about it seems nice? Are there specific use cases that exist that you're aware of that need to be able to dispatch differently? Are there specific use cases you're envisioning? How do you feel about a name change like dispatchImmediate, at least for clarity? I am just trying to understand why we would want to treat dispatches from middleware differently than dispatches from anywhere else in the code.

Comment 6

[Matt Tighe [:matt-tighe]]

Let me try to reframe my motivation for filing this bug a little bit.

An inherent complexity in our Store model (and something that is different to the JS Redux world) is that it's threaded. My hope with UiStore and the work being done in bug 1929067 is that we have the opportunity to simplify choices previously made about the Store's threading model and instead rely on more industry-common coroutine patterns and APIs that have emerged since the Store was originally implemented.

It seems like there were some assumptions about and/or issues with performance when the Store was first created. However, AFAIK these assumptions or issues were either never clearly documented, or they have been lost to time. For example, the patch that introduced MiddlewareContext.store is absolutely massive. This makes me wonder whether the introduction of some of these changes was a matter of thoughtful improvements or deadline rush.

I understand that the complexity of BrowserStore is high and that changes to it are risky - I just think it's time we re-examine some of the foundational infrastructure. This bug is not a suggestion that anything will change, but that I think we need a stronger understanding of these components. If that understanding already exists in others that are still working here, I think we should add documentation so that future readers can benefit as well.

My questions really come down to:

1. Do Stores need their own thread, or can all Stores run on the main thread and rely on middleware to launch onto different dispatchers? (I discussed this a bit here but still need to file a separate bug.)
2. Do Stores need two types dispatchers? Does this change if we can simplifying the threading model?

Comment 7

[Polly [:polly]]

this is no longer needed following the removal of this method in bug 1989622