Refactor thunks so multiple actions are never dispatched from action creators

[aeschylus]

This came up in discussion here: https://github.com/ProjectMirador/mirador/pull/2035#issuecomment-469722475, and I think a few other places. I want to create a specific thread for it here.

Thunks are a tool for managing asynchrony. There should never be multiple synchronous actions dispatched from a thunk. I think this is mostly happening in the companion window code, but I know @sdellis was working with a similar pattern in the maximise window feature branch. The preferred pattern is to have any reducers that are interested in this action receive and respond to that action. One action, one transformation assertion.

Eliminating thunks with sagas may discourage this pattern in general, as it will be impossible to dispatch an action from an action creator, but I don't think we're quite ready for that yet. For now, let's find and refactor instances of actions creators dispatching actions, and record the strategy in our contributor docs.

[charbugs]

Thunks are a tool for managing asynchrony.

This is only half the story. You CAN use thunks to managing asychrony and apparently a lot of people do so. But as the docs say:

Thunks are the recommended middleware for basic Redux side effects logic, including complex synchronous logic that needs access to the store, and simple async logic like AJAX requests.

That is, if you don't use async calls in thunks they work synchronous as every other action does.

There should never be multiple synchronous actions dispatched from a thunk.

Can you say way?

The preferred pattern is to have any reducers that are interested in this action receive and respond to that action. One action, one transformation assertion.

From what I know so far, it is simply not possible to avoid thunks (or a similar technic) if you deal with complex action logic. Take for example the closeWindow action. It includes:

• Getting the companion window IDs from window.
• Delete all companion windows.
• Delete the window.

To accomplish that you need to have a place where you have access to both the dispatch and the state. Let's look at some places that come into question:

Reducers: Doesn't work! Reducers have no access to the store at all. So the windowReducer can not dispatch delete actions to the companionWindowReducer.

React-Redux: Doesn't work too! The mapStateToProps function only receives the state while the mapDispatchToProps function only gets the dispatch passed.

Components: This works! You can inject dispatch and state (or rather some specific actions and some state pieces) into a component via the connect function of react-redux. Within the component you can then dispatch one ore more actions based on state. But this would violate "separation of concerns" because you create state logic in the component. In the case of the example the component has to know how the windows are refer to companion windows. Within the component we want to fire actions in a declarative way. The component should simply call closeWindow(windowId).

Thunks: Works! And it is basically what thunks are good for.

Eliminating thunks with sagas may discourage this pattern in general

I will take a closer look at sagas.

[charbugs]

By the way, in your picture the "Action Creators" are thunks (or similar things) because you can not call async functions within plain action creators.

[charbugs]

Ah, I think I missed something. It's not that you want drop thunks at all but only dispatch one action?

So it's something like this? [UPDATED]

const closeWindow = windowId => (dispatch, getState) => {
  const { companionWindowIds } = getState().windows[windowId];
  const action = { type: 'CLOSE_WINDOW', payload: { windowId, companionWindowIds } };
  dispatch(action);
}

[charbugs]

The main problem that I see with the current actions is that they are in-betweens. On the one hand they are often simple database actions like ADD_WINDOW and SET_CANVAS but not atomic enough to built abstractions and automation. One the other hand they are often not specific enough to provide meaningful information about user events (TOGGLE_WINDOW_SIDE_BAR does, SET_CONFIG does not).

That's why I suggested two different types of actions in the pattern I came up with: You have very basic database actions so that reducers can be created automatically, and you have combined actions that express specific user events.

I thought about the approach that you suggested and I think it can solve the problem of specific user events, even with complex logic as shown in the closeWindow thunk example from above, so :+1: to that. But there are still open points in order to make it a maintainable pattern that also matches the needs for plugin development. Most important from my perspective:

• There is still the "don't repeat youself problem" (#1926) that makes the development tedious and error prone. Here I see at least possibilities for minor refactorings that makes it bearable.

• An advanced plugin system for Mirador must provide the possibility that a plugin can write to the state of the core application. When it comes to that, plugins are forced to use the actions that are already definded in Mirador (unless you allow plugins to completely overwrite core reducers). But if we only have specific user actions (like TOGGLE_WINDOW_SIDE_BAR) plugins can not create custom actions. So I think we need to provide basic actions (create, delete, update) to allow plugins to have free write access to the state, even if we don't use them in core. (Has this point become clear, or should I provide an example?)

So, I'm basically fine with the "one action, one transformation assertion" approach as long as we can solve this points:

1. Make all actions specific user events
2. Overcome the DRY problem
3. Give plugins the possibility to freely write to state

Do you think there is a way?

(I removed the state mangement article from the wiki main page)

[charbugs]

But wait! What I said about plugins that want to write to state also applies to plugins that want to read from state.

Imagine a plugin that wants to be informed about the removal of manifests (for what reason ever). It has to listen to all the user event types that do so (e.g. REMOVE_MANIFEST_FROM_ALBUMS and RELOAD_ALBUMS). While it's possible to do it that way it is also not very sustainable. If another action gets implemented that involves removing a manifest the plugin gets stale.

In constrast if your reducers only react on basic actions the plugin can simply listen to DELETE_MANIFEST and it can rely on that because this action is always present. Actually a plugin can listen to each and every data piece with basic actions.

From the perspective of a plugin system, having basic actions in conjunction with event types (as I suggest) is really flexible. You have a high-level and a low-level API for reading and writing to state.

Did you consider the plugin question in your approach? Do you think there a other ways to match that flexibility?

[charbugs]

React-redux 7.0.0 provides an batch function for dispatching multiple actions that result in only one rerendering: https://github.com/reduxjs/react-redux/releases/tag/v7.0.0-beta.0

import { batch } from "react-redux";

function myThunk() {
  return (dispatch, getState) => {
    // should only result in one combined re-render, not two
    batch(() => {
      dispatch(increment());
      dispatch(increment());
    })
  }
}

[mejackreed]

We have moved to a saga based approach to hopefully resolve and tighten up some of this.