Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bvaughn opened this issue Dec 22, 2015 · 106 comments

Comments

@bvaughn
Copy link

bvaughn commented Dec 22, 2015

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.

@cpsubrian
Copy link

Curious if you use Immutable.js or other. In the handful of redux things I've built I couldn't imagine not using immutable, but I do have a pretty deeply nested structure that Immutable helps tame.

@bvaughn
Copy link
Author

bvaughn commented Dec 23, 2015

Wow. What an oversight for me not to mention that. Yes! We use Immutable! As you say, it's hard to imagine not using it for anything substantial.

@cpsubrian
Copy link

@bvaughn One area I've struggled with is where to draw the line between Immutable and the components. Passing immutable objects into the Components lets you use pure-render decorators/mixins very easily but then you end up with IMmutable code in your components (which I don't like). So far I have just caved and done that but I suspect you use selectors in the render() methods instead of directly accessing Immutable.js' methods?

@bvaughn
Copy link
Author

bvaughn commented Dec 23, 2015

To be honest this is something we haven't defined a hard policy on yet. Often we use selectors in our "smart" containers to extra native values from our immutable objects and then pass the native values to our components as strings, booleans, etc. Occasionally we'll pass an Immutable object but when we do- we almost always pass a Record type so that the component can treat it like a native object (with getters).

@winstonewert
Copy link

I've been moving in the opposite direction, making action creators more trivial. But I'm really just starting out with redux. Some questions about your approach:

  1. How do you test your action creators? I like moving as much logic as possible to pure, synchronous functions that don't depend on external services because its easier to test.
  2. Do you use the time travel with hot reloading? One of the neat things with with react redux devtools is that when hot reloading is setup, the store will rerun all of the actions against the new reducer. If I were to move my logic into the action creators, I'd lose that.
  3. If your action creators dispatch multiple times to bring about an effect, does that mean that your state is briefly in an invalid state? (I'm thinking here of multiple synchronous dispatches, not ones dispatch asynchronously at a later point)

@slorber
Copy link
Contributor

slorber commented Dec 27, 2015

Use selectors everywhere

Yes that seems like saying your reducers are an implementation detail of your state and that you expose your state to your component through a query API.
Like any interface it permits to decouple and make it easy to refactor the state.

Use ImmutableJS

IMO with new JS syntax it's not so much useful to use ImmutableJS anymore as you can easily modify lists and objects with normal JS. Unless you have very large lists and objects with lots of properties and you need structural sharing for performance reasons ImmutableJS is not a strict requirement.

Do more in actionCreators

@bvaughn you should really look at this project: https://github.com/yelouafi/redux-saga
When I started discussing about sagas (initially backend concept) to @yelouafi it was to solve this kind of problem. In my case I first tried to use sagas while plugging in an user onboarding on an existing app.

@bvaughn
Copy link
Author

bvaughn commented Dec 28, 2015

  1. How do you test your action creators? I like moving as much logic as possible to pure, synchronous functions that don't depend on external services because its easier to test.

I tried to describe this above, but basically... I think it makes the most sense (to me so far) to test your action-creators with a "ducks"-like approach. Begin a test by dispatching the result of an action-creator and then verify the state using selectors. This way- with a single test you can cover the action-creator, its reducer(s), and all related selectors.

  1. Do you use the time travel with hot reloading? One of the neat things with with react redux devtools is that when hot reloading is setup, the store will rerun all of the actions against the new reducer. If I were to move my logic into the action creators, I'd lose that.

No, we don't use time-travel. But why would your business logic being in an action-creator have any impact here? The only thing that updates your application's state is your reducers. And so re-running the created actions would achieve the same result either way.

  1. If your action creators dispatch multiple times to bring about an effect, does that mean that your state is briefly in an invalid state? (I'm thinking here of multiple synchronous dispatches, not ones dispatch asynchronously at a later point)

Transient invalid state is something you can't really avoid in some cases. So long as there is eventual consistency then it's usually not a problem. And again, your state could be temporarily invalid regardless of your business logic being in the action-creators or the reducers. It has more to do with side-effects and the specifics of your store.

@bvaughn
Copy link
Author

bvaughn commented Dec 28, 2015

IMO with new JS syntax it's not so much useful to use ImmutableJS anymore as you can easily modify lists and objects with normal JS. Unless you have very large lists and objects with lots of properties and you need structural sharing for performance reasons ImmutableJS is not a strict requirement.

The primary reasons for using Immutable (in my eyes) aren't performance or syntactic sugar for updates. The primary reason is that it prevents you (or someone else) from accidentally mutating your incoming state within a reducer. That's a no-no and it's unfortunately easy to do with plain JS objects.

@bvaughn you should really look at this project: https://github.com/yelouafi/redux-saga
When I started discussing about sagas (initially backend concept) to @yelouafi it was to solve this kind of problem. In my case I first tried to use sagas while plugging in an user onboarding on an existing app.

I have actually checked out that project before :) Although I haven't yet used it. It does look neat.

@winstonewert
Copy link

I tried to describe this above, but basically... I think it makes the most sense (to me so far) to test your action-creators with a "ducks"-like approach. Begin a test by dispatching the result of an action-creator and then verify the state using selectors. This way- with a single test you can cover the action-creator, its reducer(s), and all related selectors.

Sorry, I got that part. What I was wondering about is the part of tests that interacts with the asynchronicity. I might write a test something like this:

var store = createStore();
store.dispatch(actions.startRequest());
store.dispatch(actions.requestResponseReceived({...});
strictEqual(isLoaded(store.getState());

But what does your test look like? Something like this?

var mock = mockFetch();
store.dispatch(actions.request());
mock.expect("/api/foo.bar").andRespond("{status: OK}");
strictEqual(isLoaded(store.getState());

No, we don't use time-travel. But why would your business logic being in an action-creator have any impact here? The only thing that updates your application's state is your reducers. And so re-running the created actions would achieve the same result either way.

What if the code is changed? If I change the reducer, the same actions are replayed but with the new reducer. Whereas if I change the action creator, that the new versions aren't replayed. So to consider two scenarios:

With a reducer:

  1. I try an action in my app.
  2. There is a bug in my reducer, resulting in the wrong state.
  3. I fix the bug in the reducer and save
  4. Time travel loads the new reducer and puts me in the state I should have been in.

Whereas with an action creator

  1. I try an action in my app.
  2. There is a bug in the action creator, resulting in the wrong action being created
  3. I fix the bug in the action creator and save
  4. I'm still in the incorrect state, which will require me to at least try the action again, and possibly refresh if it put me in a completely broken state.

Transient invalid state is something you can't really avoid in some cases. So long as there is eventual consistency then it's usually not a problem. And again, your state could be temporarily invalid regardless of your business logic being in the action-creators or the reducers. It has more to do with side-effects and the specifics of your store.

I guess my way of thinking of redux insists that the store is always in a valid state. The reducer always takes a valid state and produces a valid state. What cases do you think forces one to allow some inconsistent states?

@sompylasar
Copy link

sompylasar commented Dec 28, 2015 via email

@slorber
Copy link
Contributor

slorber commented Dec 28, 2015

What do you mean by transient state in Redux @bvaughn and @sompylasar ? Weither the dispatch finishes, or it throws. If it throws then the state do not change.

Unless your reducer has code issues, Redux only has states that are consistent with the reducer logic. Somehow all actions dispatched are handled in a transaction: weither the whole tree updates, or the state does not change at all.

If the whole tree updates but not in an appropriate way (like a state that React can't render), it's just you don't have done your job correctly :)

In Redux the current state is to consider that a single dispatch is a transaction boundary.

However I understand the concern of @winstonewert that seems to want to dispatch 2 actions synchronously in a same transaction. Because sometimes actionCreators dispatch multiple actions and expect that all the actions are executed correctly. If 2 actions are dispatched and then the second one fail, then only the 1st one will be applied, leading to a state that we could consider "inconsistent". Maybe @winstonewert wants that if the 2nd action dispatch failes, then we rollback the 2 actions.

@winstonewert I've implemented something like that in our internal framework here and it works fine until now: https://github.com/stample/atom-react/blob/master/src/atom/atom.js
I also wanted to handle rendering errors: if a state can't be rendered successfully I wanted my state to be rollbacked to avoid blocking the UI. Unfortunatly until next release React does a very bad job when the render methods throw errors so it was not that much useful but may be in the future.

I'm pretty sure that we can allow a store to accept multiple sync dispatches in a transaction with a middleware.

However I'm not sure it would be possible to rollback the state in case of rendering error, as generally the redux store has already "commited" when we try to render its state. In my framework there is a "beforeTransactionCommit" hook that I use to trigger the rendering and to eventually rollback on any render error.

@slorber
Copy link
Contributor

slorber commented Dec 28, 2015

@gaearon I wonder if you plan to support these kind of features and if it would be possible with the current API.

It seems to me that redux-batched-subscribe does not permit to do real transaction but just reduce the number of renderings. What I see is that the store "commit" after each dispatch even if the subscription listener is only fired once at the end

@gaearon
Copy link
Contributor

gaearon commented Dec 28, 2015

Why do we need complete transaction support? I don't think I understand the use case.

@slorber
Copy link
Contributor

slorber commented Dec 28, 2015

@gaearon I'm not really sure yet but would be happy to know more of @winstonewert usecase.

The idea is that you could do dispatch([a1,a2]) and if a2 fails, then we rollback to the state before a1 was dispatched.

In the past I've often been dispatching multiple actions synchronously (on a single onClick listener for example, or in an actionCreator) and primarily implemented transactions as a way to call render only at the end of all actions being dispatched, but this has been solved in a different way by the redux-batched-subscribe project.

In my usecases the actions I used to fire on a transaction was mostly to avoid unnecessary renderings, but the actions did make sense independently so even if the dispatch failed for the 2nd action, not rollbacking the 1st action would still give me a consistent state (but maybe not the one that was planned...). I don't really know if someone can come up with a usecase where a full rollback would be useful

However when the rendering fails doesn't it make sense to try to rollback to the last state for which the render does not fail instead of trying to make progress on an unrenderable state?

@dtinth
Copy link
Contributor

dtinth commented Dec 28, 2015

Would a simple reducer enhancer work? e.g.

const enhanceReducerWithTheAbilityToConsumeMultipleActions = (reducer =>
  (state, actions) => (typeof actions.reduce === 'function'
    ? actions.reduce(reducer, state)
    : reducer(state, actions)
  )
)

With that, you can dispatch an array to the store. The enhancer unpacks individual action and feeds it to each reducer.

@gaearon
Copy link
Contributor

gaearon commented Dec 28, 2015

Yes, and it exists: https://github.com/tshelburne/redux-batched-actions

@slorber
Copy link
Contributor

slorber commented Dec 28, 2015

Ohh @gaearon I did not know that. I did not notice there were 2 distinct projects that try to solve a quite similar usecase in different ways:

Both will permit to avoid unnecessary renderings, but the 1st one would rollback all the batched actions while the 2nd one would only not apply the failing action.

@dtinth
Copy link
Contributor

dtinth commented Dec 28, 2015

@gaearon Ouch, my bad for not looking at that. 😳


Action Creators represent Impure Code

I probably haven’t had as much hands-on experience with Redux as most people, but at first sight, I have to disagree with “do more in action creators and do less in reducers,” I’ve had some similar discussion inside our company.

In Hacker Way: Rethinking Web App Development at Facebook where the Flux pattern is introduced, the very problem that leads to Flux being invented is imperative code.

In this case, an Action Creator that does I/O is that imperative code.

We’re not using Redux at work, but at where I work we used to have fine grained actions (that, of course, all make sense on its own) and trigger them in a batch. For example, when you click on a message, three actions are triggered: OPEN_MESSAGE_VIEW, FETCH_MESSAGE, MARK_NOTIFICATION_AS_READ.

Then it turns out that these “low-level” actions are no more than a “command” or “setter” or “message” to set some value inside a store. We might as well go back and use MVC and end up with simpler code if we keep doing it like this.

In a sense, Action Creators represent impure code, while Reducers (and Selectors) represent pure code. Haskell people have figured out that it’s better to have less impure code and more pure code.

For instance, in my side project (using Redux), I use webkit’s speech recognition API. It emits onresult event as you speak. There are two choices — where do these events get processed?

  • Have the action creator process the event first, then send it into the store.
  • Just send the event object into the store.

I went with number two: Just send the raw event object into the store.

However, it seems that Redux dev-tools doesn’t like it when non-plain objects are sent into the store, so I added some tiny logic in the action creator to transform these event objects into plain objects. (The code in the action creator is so trivial that it can’t go wrong.)

Then the reducer can combine these very primitive events and build up the transcript of what’s spoken. Because that logic lives inside pure code, I can very easily tweak it live (by hot-reloading the reducer).

@denis-sokolov
Copy link

I’d like to support @dtinth. Actions should represent events that happened from the real world, not how we want to react to these events. In particular, see CQRS: we want to log as much detail about a real life events, and likely the reducers will be improved in the future and process old events with new logic.

@slorber
Copy link
Contributor

slorber commented Dec 28, 2015

@dtinth @denis-sokolov I agree with you too on that. Btw when I was referencing the redux-saga project I maybe not made it clear that I'm against the idea of making the actionCreators grow and be more and more complex over time.

The Redux-saga project is also an attempt to do what you are describing @dtinth but there is a subtile difference with what you both say. It seems you want to say if you write every raw event that happened to the action log then you can easily compute any state from the reducers of this action log. This is absolutly true, and I've taken that path for a while until my app became very hard to maintain because the action log became not explicit, and reducers too complex over time.

Maybe you can look at this point of the original discussion that lead to Redux saga discussion: paldepind/functional-frontend-architecture#20 (comment)

Usecase to solve

Imagine you have a Todo app, with the obvious TodoCreated event. Then we ask you to code an app onboarding. Once the user creates a todo, we should congratulate him with a popup.

The "impure" way:

This is what @bvaughn seems to prefer

function createTodo(todo) {
   return (dispatch, getState) => {
       dispatch({type: "TodoCreated",payload: todo});
       if ( getState().isOnboarding ) {
         dispatch({type: "ShowOnboardingTodoCreateCongratulation"});
       }
   }
}

I don't like this approach because it makes the action creator highly coupled to the app view's layout. It assumes the actionCreator should know the structure of the UI state tree to take its decision.

The "compute everything from raw events" way:

This is what @denis-sokolov @dtinth seems to prefer:

function onboardingTodoCreateCongratulationReducer(state = defaultState, action) {
  var isOnboarding = isOnboardingReducer(state.isOnboarding,action);
  switch (action) {
    case "TodoCreated": 
        return {isOnboarding: isOnboarding, isCongratulationDisplayed: isOnboarding}
    default: 
        return {isOnboarding: isOnboarding, isCongratulationDisplayed: false}
  }
}

Yes you can create a reducer that knows if the congratulation should be displayed. But then you have a popup that will be displayed without even an action saying that the popup has been displayed. In my own experience doing that (and still have legacy code doing that) it is always better to make it very explicit: NEVER display the congratulation popup if no action DISPLAY_CONGRATULATION is fired. Explicit is much easier to maintain than implicit.

The simplified saga way.

The redux-saga uses generators and may look a bit complicated if you are not used to but basically with a simplified implementation you would write something like:

function createTodo(todo) {
   return (dispatch, getState) => {
       dispatch({type: "TodoCreated",payload: todo});
   }
}

function onboardingSaga(state, action, actionCreators) {
  switch (action) {
    case "OnboardingStarted": 
        return {onboarding: true, ...state};
    case "OnboardingStarted": 
        return {onboarding: false, ...state};
    case "TodoCreated": 
        if ( state.onboarding ) dispatch({type: "ShowOnboardingTodoCreateCongratulation"});
        return state;
    default: 
        return state;
  }
}

The saga is a stateful actor that receive events and may produce effects. Here it is implemented as an impure reducer to give you an idea of what it is but it actually is not in redux-saga project.

Complicating a bit the rules:

If you take care of the initial rule it is not very explicit about everything.
If you look at the above implementations, you will notice that the congratulation popup opens everytime we create a todo during the onboarding. Most likely, we want it to open only for the first created todo that happens during the onboarding and not all of them. Also, we want to allow the user to eventually redo the onboarding from the beginning.

Can you see how the code would become messy in all 3 implementations over time as the onboarding becomes more and more complicated?

The redux-saga way

With redux-saga and the above onboarding rules, you would write something like

function* onboarding() {
  while ( true ) {
    take(ONBOARDING_STARTED)
    take(TODO_CREATED)
    put(SHOW_TODO_CREATION_CONGRATULATION)
    take(ONBOARDING_ENDED)
  }
}

I think it solves this usecase in a much simpler way than the above solutions. If I'm wrong please give me your simpler implementation :)

You talked about impure code, and in this case there is no impurity in the Redux-saga implementation because the take/put effects are actually data. When take() is called it does not execute, it returns a descriptor of the effect to execute, and at some point an interpreter kicks in, so you don't need any mock to test the sagas. If you are a functional dev doing Haskell think Free / IO monads.


In this case it permits to:

  • Avoid complicating actionCreator and make it depends on getState
  • Make the implicit more explicit
  • Avoid coupling transversal logic (like the onboarding above) to your core business domain (creating todo)

It can also provide an interpretation layer, permitting to translate raw events into more meaningful/high-level events (a bit like ELM does by wrapping events as they bubble up).

Examples:

  • "TIMELINE_SCROLLED_NEAR-BOTTOM" could lead to "NEXT_PAGE_LOADED"
  • "REQUEST_FAILED" could lead to "USER_DISCONNECTED" if error code is 401.
  • "HASHTAG_FILTER_ADDED" could lead to "CONTENT_RELOADED"

If you want to achieve a modular app layout with ducks, it can permit to avoid coupling ducks together. The saga becomes the coupling point. The ducks just have to know of their raw events, and the saga interpret these raw events. This is far better than having duck1 dispatching directly actions of duck2 because it makes duck1 project more easy to reuse in another context. One could however argue that the coupling point could also be in actionCreators and this is what most people are doing today.

@dtinth
Copy link
Contributor

dtinth commented Dec 28, 2015

@slorber This is an excellent example! Thanks for taking the time to explain the benefits and drawbacks of each approach clearly. (I even think that should go in the docs.)

I used to explore a similar idea (which I named “worker components”). Basically, it’s a React component that doesn’t render anything (render: () => null), but listens to events (e.g. from stores) and triggers other side-effects. That worker component is then put inside the application root component. Just another crazy way of handling complex side effects. 😛

@bvaughn
Copy link
Author

bvaughn commented Dec 28, 2015

Lots of discussion here while I was sleeping.

@winstonewert, you raise a good point about time-travel and replaying of buggy code. I think certain types of bugs/changes won't work with time-travel either way, but I think overall you're right.

@dtinth, I'm sorry, but I'm not following along with most of your comment. Some part of your action-creator/reducer "ducks" code has to be impure, in that some part of it has to fetch data. Beyond that you lost me. One of the primary purposes of my initial post was just one of pragmatism.


@winstonewert said, "I guess my way of thinking of redux insists that the store is always in a valid state."
@slorber asked, "What do you mean by transient state in Redux @bvaughn and @sompylasar ? Weither the dispatch finishes, or it throws. If it throws then the state do not change."

I'm pretty sure we're thinking about different things. When I said "transient invalid state" I was referring to a use-case like the following. For example, myself and a colleague recently released redux-search. This search middleware listens for changes to collections of searchable things and then (re-)indexes them for search. If the user supplies filter text, redux-search returns the list of resource uids that match the user's text. So consider the following:

Imagine your application store contains a few searchable objects: [{id: 1, name: "Alex"}, {id: 2, name: "Brian"}, {id: 3, name: "Charles"}]. The user has entered filter text "e" and so the search middleware contains an array of ids 1 and 3. Now imagine that user 1 (Alex) is deleted- either in response to a user-action locally or a refresh of remote data that no longer contains that user record. At the point when your reducer updates the users collection, your store will be temporarily invalid- because redux-search will reference an id that no longer exists in the collection. Once the middleware has run again it will correct the invalid state. This sort of thing can happen anytime one node of your tree is related to another node.


@slorber said, "I don't like this approach because it makes the action creator highly coupled to the app view's layout. It assumes the actionCreator should know the structure of the UI state tree to take its decision."

I don't understand what you mean by the approach coupling the action-creator "to the app view's layout". The state tree drives (or informs) the UI. That's one of the major purposes of Flux. And your action-creators and reducers are, by definition, coupled with that state (but not with the UI).

For what it's worth the example code you wrote as something I prefer is not the sort of thing I had in mind. Maybe I did a poor job of explaining myself. I think a difficulty in discussing something like this is that it typically doesn't manifest in simple or common examples. (For example standard the TODO MVC app is not complex enough for nuanced discussions like this.)

Edited for clarity on the last point.

@bvaughn
Copy link
Author

bvaughn commented Dec 28, 2015

Btw @slorber here is an example of what I had in mind. It's a bit contrived.

Let's say your state has many nodes. One of those nodes stores shared resources. (By "shared" I mean resources that cached locally and accessed by multiple pages within your application.) These shared resources have their own action-creators and reducers ("ducks"). Another node stores information for a particular application page. Your page also has its own duck.

Let's say your page needed to load the latest and greatest Thing and then allow a user to edit it. Here's an example action-creator approach that I might use for such a situation:

import { fetchThing, thingSelector } from 'resources/thing/duck'
import { showError } from 'messages/duck'

export function fetchAndProcessThing ({ params }): Object {
  const { id } = params
  return async ({ dispatch, getState }) => {
    try {
      await dispatch(fetchThing({ id }))

      const thing = thingSelector(getState())

      dispatch({ type: 'PROCESS_THING', thing })
    } catch (err) {
      dispatch(showError(`Invalid thing id="${id}".`))
    }
  }
}

@winstonewert
Copy link

Maybe @winstonewert wants that if the 2nd action dispatch failes, then we rollback the 2 actions.

No. I wouldn't write an action creator that dispatches two actions. I'd define a single action that did two things. The OP seems to prefer action creators that dispatch smaller actions which allows the transient invalid states I dislike.

At the point when your reducer updates the users collection, your store will be temporarily invalid- because redux-search will reference an id that no longer exists in the collection. Once the middleware has run again it will correct the invalid state. This sort of thing can happen anytime one node of your tree is related to another node.

This is actually the kind of case that bothers me. To my mind, the index would ideally be something handled entirely by the reducer or a selector. Having to dispatch extra actions to keep the search up-to-date seems a less pure use of redux.

@bvaughn
Copy link
Author

bvaughn commented Dec 28, 2015

The OP seems to prefer action creators that dispatch smaller actions which allows the transient invalid states I dislike.

Not exactly. I'd favor single actions when in regard to your action-creator's node of the state-tree. But if a single, conceptual user "action" affects multiple nodes of the state-tree then you'll need to dispatch multiple actions. You can separately invoke each action (which I think is bad) or you could have a single action-creator dispatch the actions (the redux-thunk way, which I think is better because it hides that information from your view layer).

This is actually the kind of case that bothers me. To my mind, the index would ideally be something handled entirely by the reducer or a selector. Having to dispatch extra actions to keep the search up-to-date seems a less pure use of redux.

You're not dispatching extra actions. Search is a middleware. It's automatic. But there does exist a transient state when the two nodes of your tree do not agree.

@dtinth
Copy link
Contributor

dtinth commented Dec 28, 2015

@bvaughn Oh, sorry for being such a purist!

Well, impure code has to do with data fetching and other side-effects/IO, whereas pure code can not trigger any side effect. See this table for comparison between pure and impure code.

Flux best practices says that an action should “describe a user’s action, are not setters.” Flux docs also hinted further where these actions are supposed to come from:

When new data enters the system, whether through a person interacting with the application or through a web api call, that data is packaged into an action — an object literal containing the new fields of data and a specific action type.

Basically, actions are facts/data that describes “what happened,” not what should happen. Stores can only react to these actions synchronously, predictably, and without any other side effect. All other side effects should be handled in action creators (or sagas 😉).

Example

I’m not saying this is the best way or better than any other way, or even a good way. But this is what I currently consider as best practice.

For example, let’s say the user wants to view the scoreboard which requires connection to a remote server. Here’s what should happen:

  • User clicks on view scoreboard button.
  • The scoreboard view displays with a loading indicator.
  • A request is sent to the server to fetch the scoreboard.
  • Wait for the the response.
  • If it’s successful, display the scoreboard.
  • If it’s failed, the scoreboard closes and a message box appears with an error message. User can close it.
  • User can close the scoreboard.

Assuming actions can only reach the store as a result of user’s action or server response, we can create 5 actions.

  • SCOREBOARD_VIEW (as a result of user clicking the view scoreboard button)
  • SCOREBOARD_FETCH_SUCCESS (as a result of successful response from the server)
  • SCOREBOARD_FETCH_FAILURE (as a result of error response from the server)
  • SCOREBOARD_CLOSE (as a result of user clicking the close button)
  • MESSAGE_BOX_CLOSE (as a result of user clicking the close button on the message box)

These 5 actions are sufficient to handle all the requirements above. You can see that the first 4 actions have nothing to do with any "duck". Every action only describes what happened in the outside world (user wants to do this, server said that) and can be consumed by any reducer. We also don’t have MESSAGE_BOX_OPEN action, because that’s not “what happened” (although that’s what should happen).

The only way to change the state tree is to emit an action, an object describing what happened.Redux’s README

They are glued together with these action creators:

function viewScoreboard () {
  return async function (dispatch) {
    dispatch({ type: 'SCOREBOARD_VIEW' })
    try {
      const result = fetchScoreboardFromServer()
      dispatch({ type: 'SCOREBOARD_FETCH_SUCCESS', result })
    } catch (e) {
      dispatch({ type: 'SCOREBOARD_FETCH_FAILURE', error: String(e) })
    }
  }
}
function closeScoreboard () {
  return { type: 'SCOREBOARD_CLOSE' }
}

Then each part of the store (governed by reducers) can then react to these actions:

Part of Store/Reducer Behavior
scoreboardView Update visibility to true on SCOREBOARD_VIEW, false on SCOREBOARD_CLOSE and SCOREBOARD_FETCH_FAILURE
scoreboardLoadingIndicator Update visibility to true on SCOREBOARD_VIEW, false on SCOREBOARD_FETCH_*
scoreboardData Update data inside store on SCOREBOARD_FETCH_SUCCESS
messageBox Update visibility to true and store message on SCOREBOARD_FETCH_FAILURE, and update visibility to false on MESSAGE_BOX_CLOSE

As you can see, a single action can affect many parts of the store. Stores are only given high-level description of an action (what happened?) rather than a command (what to do?). As a result:

  1. It’s easier to pinpoint errors.

    Nothing can affect the state of the message box. No one can tell it to open for any reason. It only reacts to what it’s subscribed to (user actions and server responses).

    For example, if the server fails to fetch the scoreboard, and a message box did not appear, you do not need to find out why SHOW_MESSAGE_BOX action is not dispatched. It becomes obvious that the message box did not handle the SCOREBOARD_FETCH_FAILURE action properly.

    A fix is trivial and can be hot-reloaded and time-traveled.

  2. Action creators and reducers can be tested separately.

    You can test whether action creators described what happens in the outside world correctly, without any regard on how stores react to them.

    In the same way, reducers can simply be tested whether it reacts properly to the actions from the outside world.

    (An integration test would still be very useful.)

@bvaughn
Copy link
Author

bvaughn commented Dec 28, 2015

No worries. :) I appreciate the further clarification. It actually sounds like we're agreeing here. Looking at your example action-creator, viewScoreboard, it looks a lot like my example action-creator fetchAndProcessThing, right above it.

Action creators and reducers can be tested separately.

While I agree with this, I think that it often makes more pragmatic sense to test them together. It's likely that either your action or or your reducer (maybe both) are super simple and so the return-on-effort value of testing the simple one in isolation is kind of low. That's why I proposed testing the action-creator, reducer, and related selectors together (as a "duck").

@winstonewert
Copy link

But if a single, conceptual user "action" affects multiple nodes of the state-tree then you'll need to dispatch multiple actions.

That's precisely where I think what you are doing differs from what is considered best practices for redux. I think the standard way is to have one action which multiple nodes of the state tree respond to.

@bvaughn
Copy link
Author

bvaughn commented Dec 29, 2015

Ah, interesting observation @winstonewert. We've been following a pattern of using unique type-constants for each "ducks" bundle and so by extension, a reducer only response to actions dispatched by its sibling action-creators. I'm honestly not sure how I feel, initially, about arbitrary reducers responding to an action. It feels like a little like bad encapsulation.

@morgs32
Copy link

morgs32 commented Mar 16, 2017

@bvaughn can you point to any decent example of testing action, reducer, selector in the same test as you describe here?

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.

@bvaughn
Copy link
Author

bvaughn commented Mar 16, 2017

Hi @morgs32 😄

This issue is a bit old and I haven't used Redux in a while. There is a section on the Redux site about Writing Tests that you may want to check it out.

Basically I was just pointing out that- rather than writing tests for actions and reducers in isolation- it can be more efficient to write them together, like so:

import configureMockStore from 'redux-mock-store'
import { actions, selectors, reducer } from 'your-redux-module';

it('should support adding new todo items', () => {
  const mockStore = configureMockStore()
  const store = mockStore({
    todos: []
  })

  // Start with a known state
  expect(selectors.todos(store.getState())).toEqual([])

  // Dispatch an action to modify the state
  store.dispatch(actions.addTodo('foo'))

  // Verify the action & reducer worked successfully using your selector
  // This has the added benefit of testing the selector also
  expect(selectors.todos(store.getState())).toEqual(['foo'])
})

This was just my own observation after using Redux for a few months on a project. It's not an official recommendation. YMMV. 👍

@koutsenko
Copy link

"Do more in action-creators and less in reducers"

What if application is server&client and server must contain business logic and validators?
So I send action as is, and most work will be done at server-side by reducer...

@mrdulin
Copy link

mrdulin commented Nov 8, 2017

I have some different opinions.

My choice is fat reducer, thin action creators.

My action creators just dispatch actions(async, sync, serial-async, parallel-async, parallel-async in for-loop) based on some promise middleware .

My reducers split into many small state slices to handle the business logic. use combineReduers combine them. reducer is pure function, so it's easy to be re-used. Maybe someday I use angularJS, I think I can re-use my reducer in my service for the same business logic. If your reducer has many line codes, it maybe can split into smaller reducer or abstract some functions.

Yes, there are some cross-state cases in which the meanings A depend on B, C.. and B, C are async data. We must use B, C to fill or initialize A. So that's why I use crossSliceReducer.

About Do more in action-creators and less in reducers.

  1. If you use redux-thunk or etc. Yeah. You can access the complete state within action creators by getState(). This is a choice. Or, You can create some crossSliceReducer, so you can access the complete state too, you can use some state slice to compute your other state.

About Unit testing

reducer is pure function. So it's easy to be testing. For now, I just test my reducers, because it's most important than other parts.

To test action creators? I think if they are "fat", it may be not easy to be testing. Especially async action creators.

@AlastairTaft
Copy link

I agree with you @mrdulin that's now the way I've gone too.

lcarsos added a commit to lcarsos/simple-rankerrater that referenced this issue Nov 9, 2017
I'm really not liking the "rails"-style directory structure.

Now that I'm beginning to have d14rs I'm beginning to need some of the
business logic/game-ified stuff and I'm having trouble finding the right
place to plug in the impure parts, that will keep the task of ranking
from feeling like a chore.

I've read a lot of posts about how to best go about writing a redux app
that's slightly more complicated than can be delivered with the example
docs. As part of that I've learned that the shape of the state tree
should be pretty well abstracted from the rest of the application. Since
you use reducers to turn actions into state mutations, you should also
use selectors so no individual part of the UI (or you coding it) has to
care much about where your state is stored, so long as it is, and you
can deterministically get it back.

The docs suggested method of keeping your "containers" and your
"components" separate makes sense from a code standard. It's useful to
not tightly couple how you get your data to how you render the data in a
UI, so you can write tests around your storage of the data, and write
tests around rendering your UI. But, whether a node knows how to map
state, or dumbly works on props, is an implementation for THAT
component. No one up the tree, or down the tree, should have to care.
But in working on this commit, I've been bitten every time I changed a
component to be wrapped in a container, because watch updates, and I
refresh in the browser and nothing changes in the render. Going and
changing the include path for anywhere that renders a component will end
up more painful than the reminder to separate concerns is worth.

So, I think I'm going to migrate to module/domain/feature style
organization. But each component will use default export to declare
itself, and inside the file will determine whether that export is a
regular react component, or a connected one. I'm going to try to export
a destructurable { Component } so that I can write UI tests on just the
component so I don't have to find some clever way to intercept the state
mapping step, and just pass props in instead.

From reading the conversation in this pull request:
reduxjs/redux#1171
and this blog post:
http://randycoulman.com/blog/2016/09/27/modular-reducers-and-selectors/
I'm frustrated that somehow module organization means that you disregard
the recommendation to not pair your reducers and action creators. Since
the purpose of using reducers and selectors is to abstract away the
shape of the state tree as an implementation detail, it sounds to me
like it's appropriate to have a "state"/"reducer" module that handles
building and accessing your state. This may just be hubris, and I may
come back admitting my faults and wrongs.
@Vanuan
Copy link

Vanuan commented Nov 26, 2017

@mrdulin Yeah. It looks like middleware is the right place to place your impure logic.
But for business logic reducer does not seem like a right place. You'll end up with mutliple "synthetic" actions that don't represent what user has asked but what your business logic requires.

Much simpler choice is just call some pure functions/class methods from the middleware:

middleware = (...) => {
  // if(action.type == 'HIGH_LEVEL') 
  handlers[action.name]({ dispatch, params: action.payload })
}
const handlers = {
  async highLevelAction({ dispatch, params }) {
    dispatch({ loading: true });
    const data = await api.getData(params.someId);
    const processed = myPureLogic(data);
    dispatch({ loading: false, data: processed });
  }
}

@timotgl
Copy link

timotgl commented Jul 3, 2018

@bvaughn

This reduces the likelihood of mistyped variables that lead to subtle undefined values, it simplifies changes to the structure of your store, etc.

My case against using selectors everywhere, even for trivial pieces of state that don't require memoization or any sort of data transformation:

  • Shouldn't unit tests for reducers already catch mistyped state properties?
  • Changes to the structure of the store don't occur that often in my experience, mostly at the ramp-up phase of a project. Later on, if you change state.stuff.item1 to state.stuff.item2 you search through the code and change it everywhere - just like changing the name of anything else really. It's a common task and a non-brainer for people using IDEs especially.
  • Using selectors everywhere is kind of an empty abstraction. Why is there an API in-between the store and other redux code? The whole benefit of having a all application state in a simple, unified object is that is easy to access. There is also the overhead of importing/exporting the selectors everyhwere, testing them, finding the right one. I never understood this concern of accessing simple pieces of state directly. Sure, you gain consistency by having this API to access the state, but you give up on simplicity.

Obviously selectors are necessary, but I'd like to hear some other arguments for making them a mandatory API.

@IceOnFire
Copy link

IceOnFire commented Jul 3, 2018

From a practical point of view, one good reason in my experience is that libraries such as reselect or redux-saga leverage selectors to access pieces of state. This is enough for me to stick with selectors.

Philosophically speaking, I always see selectors as the "getter methods" of the functional world. And for the same reason why I would never access public attributes of a Java object, I would never access substates directly in a Redux app.

@timotgl
Copy link

timotgl commented Jul 3, 2018

@IceOnFire There's nothing to leverage if the computation isn't expensive or data transformation isn't required.

Getter methods might be common practice in Java but so is accessing POJOs directly in JS.

@sompylasar
Copy link

@timotgl

Why is there an API in-between the store and other redux code?

Selectors are a reducer's query (read) public API, actions are a reducer's command (write) public API. Reducer's structure is its implementation detail.

Selectors and actions are used in the UI layer, and in the saga layer (if you use redux-saga), not in the reducer itself.

@timotgl
Copy link

timotgl commented Jul 3, 2018

@sompylasar Not sure I follow your point of view here. There is no alternative to actions, I must use them to interact with redux. I don't have to use selectors however, I can just pick something directly from the state when it is exposed, which it is by design.

You're describing a way to think about selectors as a reducer's "read" API, but my question was what justifies making selectors a mandatory API in the first place (mandatory as in enforcing that as a best practice in a project, not by library design).

@sompylasar
Copy link

@timotgl Yes, selectors are not mandatory. But they make a good practice to prepare for future changes in the app code, making possible to refactor them separately:

  • how certain piece of state is structured and written to (the reducer concern, one place)
  • how that piece of state is used (the UI and side-effects concerns, many places where the same piece of state is queried from)

When you're about to change the store structure, without selectors you'll have to find and refactor all the places where the affected pieces of state are accessed, and this could potentially be a non-trivial task, not simply find-and-replace, especially if you pass around the fragments of state obtained directly from the store, not via a selector.

@timotgl
Copy link

timotgl commented Jul 4, 2018

@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
Copy link

@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
Copy link

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
Copy link
Contributor

@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
Copy link

timotgl commented Jul 6, 2018

@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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests