Document Redux Antipatterns

[modernserf]

The documentation covers some common showstopping problems in the troubleshooting chapter, and features many positive examples and links to examples. However, there are fewer examples of antipatterns that, while they may function, are contrary to the design goals of Redux and make things needlessly complicated.

What are some common antipatterns in Redux? And how do we want to cover them in the documentation?

[modernserf]

There are probably many documented antipatterns in these issues already, but here's a more subtle example, that I had struggled with for some time:

Bad:

function userReducer (state, action) {
    switch (action.type) {
    case UPDATE_PROFILE:
            return { ...state, user: action.payload }
    }
}

function notificationReducer (state, action) {
    switch (action.type) {
    case ADD_NOTIFICATION: 
        return { ...state, items: [...state.items, action.payload] }
    }
}

const updateProfile = (data) => (dispatch) => {
    userAPI.loadUser(data).then((user) => {
        dispatch({ type: UPDATE_PROFILE, payload: user })
        dispatch({ 
            type: ADD_NOTIFICATION, 
            payload: "Your profile has been updated." 
        })
    })
} 

Good:

function userReducer (state, action) {
    switch (action.type) {
    case UPDATE_PROFILE:
            return { ...state, user: action.payload }
    }
}

function notificationReducer (state, action) {
    switch (action.type) {
    case UPDATE_PROFILE: 
        return { ...state, 
            items: [...state.items, "Your profile has been updated."] }
    }
}

const updateProfile = (data) => (dispatch) => {
    userAPI.loadUser(data).then((user) => {
        dispatch({ type: UPDATE_PROFILE, payload: user })
    })
} 

The documentation would go on to explain how they differ, why one is preferable over the other, and where responsibilities should fall between action creators and reducers, with links to further reading in the documentation.

[ferdinandsalis]

@modernserf could you explain why its bad practice. My current signup action creator does something similar.

import { AUTH_USER } from 'redux/middleware/auth';
import { pushState } from 'redux-router';
import { setFlash, setErrorFlash } from 'redux/actions/flash';
import { SIGNUP_REQUEST, SIGNUP_SUCCESS, SIGNUP_FAILURE } from 'redux/constants';

export function signup(data) {
  return {
    [AUTH_USER]: { types: [SIGNUP_REQUEST, SIGNUP_SUCCESS, SIGNUP_FAILURE] },
    payload: (client) => client.signup(data)
  };
}

export default function signupUser(data) {
  return dispatch => {
    dispatch(signup(data)).then(action => {
      if (action.type === SIGNUP_SUCCESS) {
        dispatch(pushState(null, '/account'));
        dispatch(setFlash({
          message: 'Yeah, you successfully signed up.',
          kind: 'success'
        }));
      }
    }, err => {
      dispatch(setErrorFlash());
      console.error(err);
    });
  }
}

[gaearon]

I wouldn't actually call that an anti-pattern, it's a fine thing to do in many cases.

[modernserf]

• Redux actions are broadcast to all reducers; ACs should take advantage of this instead of being treated like the API for a single reducer
• ACs are largely a way of formatting data and handling side effects; the lions share of the logic should be in the reducers, not the ACs
• Its much more likely that you will test your reducers than your ACs
• an AC that dispatches another AC couples them together and adds an unnecessary layer of indirection

The "ducks" model (which I was using until I had this revelation) bundles action creators, action types and reducers together, and leads you to the top style -- which works! Plus its more intuitive when you're coming from other event-oriented systems.

But its fighting against Redux's nature: actions are broadcast; every action goes to every reducer. SIGNUP_REQUEST, SIGNUP_SUCCESS and SIGNUP_FAILURE all map to side effects introduced by the user or an AJAX request; they're fairly high-level explanations of things that are happening to your app. ADD_NOTIFICATION is a low-level instruction that doesn't map to any particular action performed by the user or the network.

I recognize that this is largely a matter of taste, but this has helped me wrap my head around the Redux model.

[ferdinandsalis]

@modernserf thanks for clarifying this. Now I understand.

[tomazzaman]

@ferdinandsalis I have a challenge for you, and @gaearon as well - since this appears in the real world example.

Provide a unit test for this piece of code (provided AUTH_USER is a Symbol):

export function signup(data) {
  return {
    [AUTH_USER]: { types: [SIGNUP_REQUEST, SIGNUP_SUCCESS, SIGNUP_FAILURE] },
    payload: (client) => client.signup(data)
  };
}

Requirement: it should run in Karma/PhantomJS

I have wasted spent days on this.

[gaearon]

@tomazzaman

What exactly is it that you want to test here?
That it runs a request given the middleware?
That it correctly describes the request?

I guess running expect on the return values is not what you want, but then you need to explain what you want to test exactly. Here's an example without the middleware: #546 (comment).

Finally, is there a particular problem in using the Symbol? Well, there's no need to use it if you have problems with Symbols! It's just code. Change it how you like. No need to follow examples word by word. ;-)

[gaearon]

For example I'm not sure this is really a helpful test as it's just duplicating the code:

import { AUTH_USER } from '../middleware/api';

let asyncAction = signup(data);
expect(asyncAction[AUTH_USER].types).toEqual([SIGNUP_REQUEST, SIGNUP_SUCCESS, SIGNUP_FAILURE]);

If it's useful, that's the answer to your question. If not, then what you really want to test is either the middleware itself, or middleware executing your particular action.

Here's an example of doing the latter:

import apiMiddleware from '../middleware/api';

function createMockStore(mockState, expectedActions, done) {
  return applyMiddleware(apiMiddleware)(() => {
    getState() {
      return mockState;
    },

    dispatch(action) {
      const expectedAction = expectedActions.shift();
      expect(action).toEqual(expectedAction);

      if (!expectedActions.length) {
        done();
      }
    }
  })();
}

describe('action creators', () => {
  afterEach(() => {
    nock.cleanAll();
  });

  it('creates signup actions', (done) => {
    // mock requests you need in this test
    nock('http://localhost:3000/')
      .get('/signup')
      .reply(200);

    const expectedActions = [
      { type: 'SIGNUP_REQUEST' },
      { type: 'SIGNUP_SUCCESS', payload: { /* ... */ } }
    ];
    const mockState = {
      something: 42
    };

    const mockStore = createMockStore(mockState, expectedActions, done);
    mockStore.dispatch(signup());
  });
});

[gaearon]

PS @tomazzaman please file a separate issue next time because this is unrelated to the topic.
This issue is about anti-patterns, not about testing action creators.

[tomazzaman]

Thanks @gaearon! I have absolutely no experience with Symbols in JavaScript (but use it in Ruby all the time) and that led to console.log( [object with symbol as a key] ) appearing as an empty object in all browsers but Chrome, when in fact, it was not empty which I was able to verify with Object.getOwnPropertySymbols(response); literally minutes after I posted this question. In any case, I'm happy I did, because your example above should definitely be in the Redux testing documentation, so someone else might stumble upon it and thank both of us :)

In truth, I'd prefer all the constants to be Symbols (with Symbol.for('Call API')), because it's easy to access them anywhere, so my unit test looks something like this:

// actions.js
import { CALL_API } from '../middleware/api';

export const AUTH_LOGIN = 'AUTH_LOGIN';
export const AUTH_LOGIN_SUCCESS = 'AUTH_LOGIN_SUCCESS';
export const AUTH_LOGIN_FAILURE = 'AUTH_LOGIN_FAILURE';

export function login( user ) {
  return {
    [CALL_API]: {
      types: [AUTH_LOGIN, AUTH_LOGIN_SUCCESS, AUTH_LOGIN_FAILURE],
      endpoint: 'users/login',
      request: {
        method: 'POST',
        body: user,
      },
    },
  };
}

// actions.spec.js
import { expect } from 'chai';
import { login } from '../../../src/js/actions/authActions.js';

const key = Symbol.for('Call API');

describe.only( 'authActions', () => {
  it( 'should dispatch a proper login action', () => {
    const user = { email: 'peter@klaven', password: 'cityslicka' };
    const response = login( user );
    const value = response[ key ];

    expect( value.types ).to.include(  'AUTH_LOGIN_SUCCESS' );
    expect( value.endpoint ).to.equal( 'users/login' );
  } );
} );

PS: Sorry for abusing this topic, I didn't think my question was a separate topic worthy :) Will do so next time.

[exogen]

It's true that @modernserf's antipattern example is fine to do in many cases, but IMO it's never a bad idea to offer some official guidance with respect to best practices. I realize @gaearon might think this makes Redux seem more opinionated whereas he likes that it's so simple and open-ended – but again, guidance is always good!

(In my own usage I've arrived at the same conclusion as @modernserf – in that example, only one action took place. Adding the notification was a state modification in response to that single action, not a separate action in and of itself.)

[exogen]

I'll add that even if Redux doesn't take a stance one way or the other, merely mentioning both patterns in a FAQ or whatever is going to help a lot of people.

[acstll]

@modernserf thanks for sharing, though in my old vanilla Flux code I had a Notifications store that would react to all kind of actions (just like in your "Good" example), in my recent Redux code I've been making this mistake of having a notify action creator, for example.

I have a question for you. How do you clear up your notifications (or flash messages @ferdinandsalis) after they've showed up?

I have a case where I set a timer inside the notifications component just to hide it… but I'm not sure about that. I would like my notifications to stack inside the store and then be cleared up as they show…

[ferdinandsalis]

@acstll to hide the message again I setup a setTimeout within componentWillReceiveProps that dispatches a FLASH_HIDE action.

I am not sure what you mean by …

I would like my notifications to stack inside the store and then be cleared up as they show…

[acstll]

@ferdinandsalis that's exactly how I do it now.

I meant to have more than one notification (or flash message) in the store, namely an array of strings. So I can display them one after another, each of them keeping it's entire "display time".

Imagine in an app there's a list of things a user can delete and you show a notification for every deleted item. If your timer is set to 2s and the user deletes a few in a row very quickly, and something else that requires a notification happens in between, it gets messy. Don't you have that problem with your current set-up?

[gaearon]

That's exactly the use case where I also fire separate actions.

See my answer on StackOverflow about when to use separate actions: http://stackoverflow.com/a/33226443/458193

[modernserf]

@acstll I have actions to mark a notification as viewed (fired on mount) and an action to dismiss it (used with a close button.)

I'll add that even if Redux doesn't take a stance one way or the other, merely mentioning both patterns in a FAQ or whatever is going to help a lot of people.

This is probably the right idea. My goal for this ticket wasn't to highlight this particular pattern; rather I wanted to discuss how we would collect some of this shared knowledge. The docs do a very good job for a library as young as it is, but a lot of the community knowledge, particularly around design decisions like these, are documented in github issues and stack overflow questions. I'm not sure where, how, or even if these should be in the official documentation, but I felt like it was worth asking.

[pe3]

Collecting some links:

Dan on Twitter: "Specifying the initial state as an object literal when creating a Redux store is an anti-pattern".

Dan on Twitter: "Most common Redux misconception: reducers and action creators are a 1:1 mapping".

[gaearon]

https://twitter.com/dan_abramov/status/688087202312491008

“Common Redux misconception: you need to deeply clone the state. Reality: if something inside doesn’t change, keep its reference the same!”

[modernserf]

Big ball-of-mud reducers: https://twitter.com/dan_abramov/status/689832524156116992
I'm not exactly sure what the preferred pattern is here, though -- is it that reducers should operate on the smallest possible amount of state?

[gaearon]

Not exactly the smallest, but it's best when things that don't change together are separated. For example if you're updating a todo list, it's best to separate the logic for updating the array and updating the items inside it.

[gaearon]

Coupling reducers to actions and using classes for state: https://medium.com/@scrapcupcake/so-i-ve-been-learning-dan-abramov-s-redux-library-via-the-egghead-io-40ca26b6e9ed

[markerikson]

Currently working on an FAQ list over in #1312 . Several of these items will probably wind up in there. Further suggestions appreciated.

[gaearon]

Relevant http://stackoverflow.com/questions/37738268/how-to-avoid-race-conditions-when-fetching-data-with-redux/37738420#37738420

[markerikson]

This is mostly covered via the FAQ and the "Structuring Reducers" docs sections. If anyone wants to submit updates to the docs with more details, PRs are welcome.

[euZebe]

May I suggest another anti-pattern:

function thingReducer(state =  {}, action) {
  switch (action.type) {
    case 'SOME_ACTION_ABOUT_PANELS':
    case 'ANOTHER_ACTION_ABOUT_PANELS':
      return {
        ...state,
        panels: panelsReducer(state.panels, action), // panelsReducer dealing effectively with those actions
      };
  }
}

function panelsReducer(panelsState = {}, action) {
  switch(action.type) {
    case 'SOME_ACTION_ABOUT_PANELS':
      return ... 
    case 'ANOTHER_ACTION_ABOUT_PANELS':
      return ... 
  }
}

Make an action not being broadcasted to every reducer...

[markerikson]

@euZebe : that doesn't look like an anti-pattern to me at all. It's explicit update logic that is delegating the work of updating state.panels to panelsReducers for those specific cases.

The idea that "all actions are broadcast to all reducers" is only true when you're using combineReducers. It helps to remember that you really only have one reducer function - your root reducer. How that root reducer splits up the work internally is totally up to you.

[euZebe]

Ok, then that's just something I had not seen before. I updated my example to explain a bit more why I didn't like it: if I want to add a new action type, I need to add a case in panelsReducer, but also in thingReducer just for the action to be routed to the sub-reducer.

[markerikson]

Yep, as I said, that's being explicit about the update logic. If you want to delegate the work of updating things.panels automatically, use combineReducers() to generate the things reducer instead.