Recommendations for best practices regarding action-creators, reducers, and selectors

[bvaughn]

My team has been using Redux for a couple of months now. Along the way I've occasionally found myself thinking about a feature and wondering "does this belong in an action-creator or a reducer?". The documentation seems a bit vague on this fact. (Or perhaps I've just missed where it's covered, in which case I apologize.) But as I've written more code and more tests I've come to have stronger opinions about where things should be and I thought it would be worth sharing and discussing with others.

So here are my thoughts.

Use selectors everywhere

This first one is not strictly related to Redux but I'll share it anyway since it's indirectly mentioned below. My team uses rackt/reselect. We typically define a file that exports selectors for a given node of our state tree (eg. MyPageSelectors). Our "smart" containers then use those selectors to parameterize our "dumb" components.

Over time we've realized that there is added benefit to using these same selectors in other places (not just in the context of reselect). For example, we use them in automated tests. We also use them in thunks returned by action-creators (more below).

So my first recommendation is- use shared selectors everywhere- even when synchronously accessing data (eg. prefer myValueSelector(state) over state.myValue). This reduces the likelihood of mistyped variables that lead to subtle undefined values, it simplifies changes to the structure of your store, etc.

Do more in action-creators and less in reducers

I think this one is very important although it may not be immediately obvious. Business logic belongs in action-creators. Reducers should be stupid and simple. In many individual cases it does not matter- but consistency is good and so it's best to consistently do this. There are a couple of reasons why:

1. Action-creators can be asynchronous through the use of middleware like redux-thunk. Since your application will often require asynchronous updates to your store- some "business logic" will end up in your actions.
2. Action-creators (more accurately the thunks they return) can use shared selectors because they have access to the complete state. Reducers cannot because they only have access to their node.
3. Using redux-thunk, a single action-creator can dispatch multiple actions- which makes complicated state updates simpler and encourages better code reuse.

Imagine your state has metadata related to a list of items. Each time an item is modified, added to, or removed from the list- the metadata needs to be updated. The "business logic" for keeping the list and its metadata in sync could live in a few places:

1. In the reducers. Each reducer (add, edit, remove) is responsible for updating the list as well as the metadata.
2. In the views (container/component). Each view that invokes an action (add, edit, remove) it is also responsible for invoking an updateMetadata action. This approach is terrible for (hopefully) obvious reasons.
3. In the action-creators. Each action-creator (add, edit, remove) returns a thunk that dispatches an action to update the list and then another action to updates the metadata.

Given the above choices, option 3 is solidly better. Both options 1 and 3 support clean code sharing but only option 3 supports the case where list and/or metadata updates might be asynchronous. (For example maybe it relies on a web worker.)

Write "ducks" tests that focus on Actions and Selectors

The most efficient way to tests actions, reducers, and selectors is to follow the "ducks" approach when writing tests. This means you should write one set of tests that cover a given set of actions, reducers, and selectors rather than 3 sets of tests that focus on each individually. This more accurately simulates what happens in your real application and it provides the most bang for the buck.

Breaking it down further I've found that it's useful to write tests that focus on action-creators and then verify the outcome using selectors. (Don't directly test reducers.) What matters is that a given action results in the state you expect. Verifying this outcome using your (shared) selectors is a way of covering all three in a single pass.

[timotgl]

@sompylasar Thanks for your input.

this could potentially be a non-trivial task

That's a valid concern, it just seems like an expensive tradeoff to me. I guess I haven't come across a state refactoring that caused such problems. I have come across "selector spaghetti" however, where nested selectors for every trivial sub-piece of state caused quite the confusion. This counter-measure itself has to be maintained as well after all. But I understand the reason behind this better now.

[sompylasar]

@timotgl A simple example that I can share publicly:

export const PROMISE_REDUCER_STATE_IDLE = 'idle';
export const PROMISE_REDUCER_STATE_PENDING = 'pending';
export const PROMISE_REDUCER_STATE_SUCCESS = 'success';
export const PROMISE_REDUCER_STATE_ERROR = 'error';

export const PROMISE_REDUCER_STATES = [
  PROMISE_REDUCER_STATE_IDLE,
  PROMISE_REDUCER_STATE_PENDING,
  PROMISE_REDUCER_STATE_SUCCESS,
  PROMISE_REDUCER_STATE_ERROR,
];

export const PROMISE_REDUCER_ACTION_START = 'start';
export const PROMISE_REDUCER_ACTION_RESOLVE = 'resolve';
export const PROMISE_REDUCER_ACTION_REJECT = 'reject';
export const PROMISE_REDUCER_ACTION_RESET = 'reset';

const promiseInitialState = { state: PROMISE_REDUCER_STATE_IDLE, valueOrError: null };
export function promiseReducer(state = promiseInitialState, actionType, valueOrError) {
  switch (actionType) {
    case PROMISE_REDUCER_ACTION_START:
      return { state: PROMISE_REDUCER_STATE_PENDING, valueOrError: null };
    case PROMISE_REDUCER_ACTION_RESOLVE:
      return { state: PROMISE_REDUCER_STATE_SUCCESS, valueOrError: valueOrError };
    case PROMISE_REDUCER_ACTION_REJECT:
      return { state: PROMISE_REDUCER_STATE_ERROR, valueOrError: valueOrError };
    case PROMISE_REDUCER_ACTION_RESET:
      return { ...promiseInitialState };
    default:
      return state;
  }
}

export function extractPromiseStateEnum(promiseState = promiseInitialState) {
  return promiseState.state;
}
export function extractPromiseStarted(promiseState = promiseInitialState) {
  return (promiseState.state === PROMISE_REDUCER_STATE_PENDING);
}
export function extractPromiseSuccess(promiseState = promiseInitialState) {
  return (promiseState.state === PROMISE_REDUCER_STATE_SUCCESS);
}
export function extractPromiseSuccessValue(promiseState = promiseInitialState) {
  return (promiseState.state === PROMISE_REDUCER_STATE_SUCCESS ? promiseState.valueOrError || null : null);
}
export function extractPromiseError(promiseState = promiseInitialState) {
  return (promiseState.state === PROMISE_REDUCER_STATE_ERROR ? promiseState.valueOrError || true : null);
}

It's not this reducer user's concern whether what's being stored is state, valueOrError or something else. Exposed are the state string (enum), a couple frequently used checks on that state, the value and the error.

I have come across "selector spaghetti" however, where nested selectors for every trivial sub-piece of state caused quite the confusion.

If this nesting was caused by mirroring the reducer nesting (composition), that's not what I'd recommend, that's that reducer's implementation detail. Using the above example, the reducers that use promiseReducer for some parts of their state export their own selectors named according to these parts. Also not every function that looks like a selector has to be exported and be part of the reducer API.

function extractTransactionLogPromiseById(globalState, transactionId) {
  return extractState(globalState).transactionLogPromisesById[transactionId] || undefined;
}

export function extractTransactionLogPromiseStateEnumByTransactionId(globalState, transactionId) {
  return extractPromiseStateEnum(extractTransactionLogPromiseById(globalState, transactionId));
}

export function extractTransactionLogPromiseErrorTransactionId(globalState, transactionId) {
  return extractPromiseError(extractTransactionLogPromiseById(globalState, transactionId));
}

export function extractTransactionLogByTransactionId(globalState, transactionId) {
  return extractPromiseSuccessValue(extractTransactionLogPromiseById(globalState, transactionId));
}

[sompylasar]

Oh one more thing that I almost forgot. Function names and exports/imports can be minified well and safely. Nested object keys — not so much, you need proper data flow tracer in the minifier to not screw up the code.

[markerikson]

@timotgl : a lot of our encouraged best practices with Redux are about trying to encapsulate Redux-related logic and behavior.

For example, you can dispatch actions directly from a connected component, by doing this.props.dispatch({type : "INCREMENT"}). However, we discourage that, because it forces the component to "know" it's talking to Redux. A more React-idiomatic way to do things is to pass in bound action creators, so that the component can just call this.props.increment(), and it doesn't matter whether that function is a bound Redux action creator, a callback passed down by a parent, or a mock function in a test.

You can also hand-write action types everywhere, but we encourage defining constant variables so that they can be imported, traced, and decrease the chance of typos.

Similarly, there's nothing preventing you from accessing state.some.deeply.nested.field in your mapState functions or thunks. But, as has already been described in this thread, this increases the chances of typos, makes it harder to track down places that a particular piece of state is being used, makes refactoring more difficult, and means that any expensive transformation logic is probably re-running every time even if it doesn't need to.

So no, you don't have to use selectors, but they're a good architectural practice.

You might want to read through my post Idiomatic Redux: Using Reselect Selectors for Encapsulation and Performance.

[timotgl]

@markerikson I'm not arguing against selectors in general, or selectors for expensive computations. And I never intended to pass dispatch itself to a component :)

My point was that I disagree with this belief:

Ideally, only your reducer functions and selectors should know the exact state structure, so if you change where some state lives, you would only need to update those two pieces of logic.

About your example with state.some.deeply.nested.field, I can see the value of having a selector to shorten this. selectSomeDeeplyNestedField() is one function name vs. 5 properties I could get wrong.

On other hand, if you follow this guideline to the letter, you're also doing const selectSomeField = state => state.some.field; or even const selectSomething = state => state.something;, and at some point the overhead (import, export, testing) of doing this consistently doesn't justify the (debatable) safety and purity anymore in my opinion. It's well-meant but I can't shake off the dogmatic spirit of the guideline. I'd trust the developers in my project to use selectors wisely and when applicable - because my experience so far has been that they do that.

Under certain conditions I could see why you would want to err on the side of safety and conventions though. Thanks for your time and engagement, I'm overall very happy we have redux.

[markerikson]

Sure. FWIW, there are other selector libraries, and also wrappers around Reselect as well. For example, https://github.com/planttheidea/selectorator lets you define dot-notation key paths, and it does the intermediate selectors for you internally.