Skip to content

Trying to put API calls in the correct place #291

Closed
@5h1rU

Description

@5h1rU

I'm trying to make a login success/error flow but my main concern is where I can put this logic.

Currently I'm using actions -> reducer (switch case with action calling API) -> success/error on response triggering another action.

The problem with this approach is that the reducer is not working when I call the action from the API call.

am I missing something?

Reducer

import { LOGIN_ATTEMPT, LOGGED_FAILED, LOGGED_SUCCESSFULLY } from '../constants/LoginActionTypes';
import Immutable from 'immutable';
import LoginApiCall from '../utils/login-request';

const initialState = new Immutable.Map({
  email: '',
  password: '',
}).asMutable();

export default function user(state = initialState, action) {
  switch (action.type) {
    case LOGIN_ATTEMPT:
      console.log(action.user);
      LoginApiCall.login(action.user);
      return state;
    case LOGGED_FAILED:
      console.log('failed from reducer');
      return state;
    case LOGGED_SUCCESSFULLY:
      console.log('success', action);
      console.log('success from reducer');
      break;
    default:
      return state;
  }
}

actions

import { LOGIN_ATTEMPT, LOGGED_FAILED, LOGGED_SUCCESSFULLY } from '../constants/LoginActionTypes';

export function loginError(error) {
  return dispatch => {
    dispatch({ error, type: LOGGED_FAILED });
  };
}

/*
 * Should add the route like parameter in this method
*/
export function loginSuccess(response) {
  return dispatch => {
    dispatch({ response, type: LOGGED_SUCCESSFULLY });
    // router.transitionTo('/dashboard'); // will fire CHANGE_ROUTE in its change handler
  };
}

export function loginRequest(email, password) {
  const user = {email: email, password: password};
  return dispatch => {
    dispatch({ user, type: LOGIN_ATTEMPT });
  };
}

API calls

 // Use there fetch polyfill
 // The main idea is create a helper in order to handle success/error status
import * as LoginActions from '../actions/LoginActions';

const LoginApiCall = {
  login(userData) {
    fetch('http://localhost/login', {
      method: 'post',
      headers: {
        'Accept': 'application/json',
        'Content-Type': 'application/json',
      },
      body: JSON.stringify({
        email: userData.email,
        password: userData.password,
      }),
    })
    .then(response => {
      if (response.status >= 200 && response.status < 300) {
        console.log(response);
        LoginActions.loginSuccess(response);
      } else {
        const error = new Error(response.statusText);
        error.response = response;
        LoginActions.loginError();
        throw error;
      }
    })
    .catch(error => { console.log('request failed', error); });
  },
};

export default LoginApiCall;

Activity

gaearon

gaearon commented on Jul 20, 2015

@gaearon
Contributor

This is almost correct, but the problem is you can't just call pure action creators and expect things to happen. Don't forget your action creators are just functions that specify what needs to be dispatched.

// CounterActions
export function increment() {
  return { type: INCREMENT }
}


// Some other file
import { increment } from './CounterActions';
store.dispatch(increment()); <--- This will work assuming you have a reference to the Store
increment(); <---- THIS DOESN'T DO ANYTHING! You're just calling your function and ignoring result.


// SomeComponent
import { increment } from './CounterActions';

@connect(state => state.counter) // will inject store's dispatch into props
class SomeComponent {
  render() {
    return <OtherComponent {...bindActionCreators(CounterActions, this.props.dispatch)} />
  }
}


// OtherComponent
class OtherComponent {
  handleClick() {
    // this is correct:
    this.props.increment(); // <---- it was bound to dispatch in SomeComponent

    // THIS DOESN'T DO ANYTHING:
    CounterActions.increment(); // <---- it's just your functions as is! it's not bound to the Store.
  }
}
gaearon

gaearon commented on Jul 20, 2015

@gaearon
Contributor

Now, let's get to your example. The first thing I want to clarify is you don't need async dispatch => {} form if you only dispatch a single action synchronously and don't have side effects (true for loginError and loginRequest).

This:

import { LOGIN_ATTEMPT, LOGGED_FAILED, LOGGED_SUCCESSFULLY } from '../constants/LoginActionTypes';

export function loginError(error) {
  return dispatch => {
    dispatch({ error, type: LOGGED_FAILED });
  };
}

export function loginSuccess(response) {
  return dispatch => {
    dispatch({ response, type: LOGGED_SUCCESSFULLY });
    // router.transitionTo('/dashboard');
  };
}

export function loginRequest(email, password) {
  const user = {email: email, password: password};
  return dispatch => {
    dispatch({ user, type: LOGIN_ATTEMPT });
  };
}

can be simplified as

import { LOGIN_ATTEMPT, LOGGED_FAILED, LOGGED_SUCCESSFULLY } from '../constants/LoginActionTypes';

export function loginError(error) {
  return { error, type: LOGGED_FAILED };
}

// You'll have a side effect here so (dispatch) => {} form is a good idea
export function loginSuccess(response) {
  return dispatch => {
    dispatch({ response, type: LOGGED_SUCCESSFULLY });
    // router.transitionTo('/dashboard');
  };
}

export function loginRequest(email, password) {
  const user = {email: email, password: password};
  return { user, type: LOGIN_ATTEMPT };
}
gaearon

gaearon commented on Jul 20, 2015

@gaearon
Contributor

Secondly, your reducers are supposed to be pure functions that don't have side effects. Do not attempt to call your API from a reducer. Reducer is synchronous and passive, it only modifies the state.

This:

const initialState = new Immutable.Map({
  email: '',
  password: '',
  isLoggingIn: false,
  isLoggedIn: false,
  error: null
}).asMutable(); // <---------------------- why asMutable?

export default function user(state = initialState, action) {
  switch (action.type) {
    case LOGIN_ATTEMPT:
      console.log(action.user);
      LoginApiCall.login(action.user); // <------------------------ no side effects in reducers! :-(
      return state;
    case LOGGED_FAILED:
      console.log('failed from reducer');
      return state;
    case LOGGED_SUCCESSFULLY:
      console.log('success', action);
      console.log('success from reducer');
      break;
    default:
      return state;
  }

should probably look more like

const initialState = new Immutable.Map({
  email: '',
  password: '',
  isLoggingIn: false,
  isLoggedIn: false,
  error: null
});

export default function user(state = initialState, action) {
  switch (action.type) {
    case LOGIN_ATTEMPT:
      return state.merge({
        isLoggingIn: true,
        isLoggedIn: false,
        email: action.email,
        password: action.password // Note you shouldn't store user's password in real apps
      });
    case LOGGED_FAILED:
      return state.merge({
        error: action.error,
        isLoggingIn: false,
        isLoggedIn: false
      });
    case LOGGED_SUCCESSFULLY:
      return state.merge({
        error: null,
        isLoggingIn: false,
        isLoggedIn: true
      });
      break;
    default:
      return state;
  }
gaearon

gaearon commented on Jul 20, 2015

@gaearon
Contributor

Finally, where do you put the login API call then?

This is precisely what dispatch => {} action creator is for. Side effects!

It's just another action creator. Put it together with other actions:

import { LOGIN_ATTEMPT, LOGGED_FAILED, LOGGED_SUCCESSFULLY } from '../constants/LoginActionTypes';

export function loginError(error) {
  return { error, type: LOGGED_FAILED };
}

export function loginSuccess(response) {
  return dispatch => {
    dispatch({ response, type: LOGGED_SUCCESSFULLY });
    router.transitionTo('/dashboard');
  };
}

export function loginRequest(email, password) {
  const user = {email: email, password: password};
  return { user, type: LOGIN_ATTEMPT };
}

export function login(userData) {
  return dispatch =>
    fetch('http://localhost/login', {
      method: 'post',
      headers: {
        'Accept': 'application/json',
        'Content-Type': 'application/json',
      },
      body: JSON.stringify({
        email: userData.email,
        password: userData.password,
      }),
    })
    .then(response => {
      if (response.status >= 200 && response.status < 300) {
        console.log(response);
        dispatch(loginSuccess(response));
      } else {
        const error = new Error(response.statusText);
        error.response = response;
        dispatch(loginError(error));
        throw error;
      }
    })
    .catch(error => { console.log('request failed', error); });
}

In your components, just call

this.props.login(); // assuming it was bound with bindActionCreators before

// --or--

this.props.dispatch(login()); // assuming you only have dispatch from Connector
gaearon

gaearon commented on Jul 20, 2015

@gaearon
Contributor

Finally, if you find yourself often writing large action creators like this, it's a good idea to write a custom middleware for async calls compatible with promises are whatever you use for async.

The technique I described above (action creators with dispatch => {} signature) is right now included in Redux, but in 1.0 will be available only as a separate package called redux-thunk. While you're at it, you might as well check out redux-promise-middleware or redux-promise.

dariocravero

dariocravero commented on Jul 20, 2015

@dariocravero
Contributor

@gaearon 👏 that was an amazing explanation! ;) It should definitely make it into the docs.

andreychev

andreychev commented on Jul 20, 2015

@andreychev

@gaearon awesome explanation! 🏆

tomkis

tomkis commented on Jul 20, 2015

@tomkis
Contributor

@gaearon What if you need to perform some logic to determine which API call should be called? Isn't it domain logic which should be in one place(reducers)? Keeping that in Action creators sounds to me like breaking single source of truth. Besides, mostly you need to parametrise the API call by some value from the application state. You most likely also want to test the logic somehow. We found making API calls in Reducers (atomic flux) very helpful and testable in large scale project.

gaearon

gaearon commented on Jul 20, 2015

@gaearon
Contributor

We found making API calls in Reducers (atomic flux) very helpful and testable in large scale project.

This breaks record/replay. Functions with side effects are harder to test than pure functions by definition. You can use Redux like this but it's completely against its design. :-)

Keeping that in Action creators sounds to me like breaking single source of truth.

“Single source of truth” means data lives in one place, and has no independent copies. It doesn't mean “all domain logic should be in one place”.

What if you need to perform some logic to determine which API call should be called? Isn't it domain logic which should be in one place(reducers)?

Reducers specify how state is transformed by actions. They shouldn't worry about where these actions originate. They may come from components, action creators, recorded serialized session, etc. This is the beauty of the concept of using actions.

Any API calls (or logic that determines which API is called) happens before reducers. This is why Redux supports middleware. Thunk middleware described above lets you use conditionals and even read from the state:

// Simple pure action creator
function loginFailure(error) {
  return { type: LOGIN_FAILURE, error };
}

// Another simple pure action creator
function loginSuccess(userId) {
  return { type: LOGIN_SUCCESS, userId };
}

// Another simple pure action creator
function logout() {
  return { type: LOGOUT };  
}


// Side effect: uses thunk middleware
function login() {
  return dispatch => {
    MyAwesomeAPI.performLogin().then(
      json => dispatch(loginSuccess(json.userId)),
      error => dispatch(loginFailure(error))
    )
  };
}


// Side effect *and* reads state
function toggleLoginState() {
  return (dispatch, getState) => {
    const { isLoggedIn } = getState().loginState;
    if (isLoggedIn) {
      dispatch(login());
    } else {
      dispatch(logout());
    }
  };
}

// Component
this.props.toggleLoginState(); // Doesn't care how it happens

Action creators and middleware are designed to perform side effects and complement each other.
Reducers are just state machines and have nothing to do with async.

tomkis

tomkis commented on Jul 20, 2015

@tomkis
Contributor

Reducers specify how state is transformed by actions.

This is indeed very valid point, thanks.

gaearon

gaearon commented on Jul 20, 2015

@gaearon
Contributor

I'll reopen for posterity until a version of this is in the docs.

104 remaining items

gaearon

gaearon commented on Jan 8, 2016

@gaearon
Contributor

No, this wasn't my point. Take a look at makeSandwichesForEverybody. It's a thunk action creator calling other thunk action creators. This is why you don't need to put everything in components. See also async example in this repo for more of this.

catamphetamine

catamphetamine commented on Jan 8, 2016

@catamphetamine

@gaearon But I think it wouldn't be appropriate to put, say, my fancy animation code into the action creator, would it?
Consider this example:

button_on_click()
{
    this.props.dispatch(load_stuff())
        .then(() =>
        {
                const modal = ReactDOM.getDOMNode(this.refs.modal)
                jQuery(modal).fancy_animation(1200 /* ms */)
                setTimeout(() => jQuery.animate(modal, { background: 'red' }), 1500 /* ms */)
        })
        .catch(() =>
        {
                alert('Failed to bypass the root mainframe protocol to override the function call on the hyperthread's secondary firewall')
        })
}

How would you rewrite it the Right way?

slorber

slorber commented on Jan 8, 2016

@slorber
Contributor

@halt-hammerzeit you can make the actionCreator return the promises so that the component can show some spinner or whatever by using local component state (hard to avoid when using jquery anyway)

Otherwise you can manage complex timers to drive animations with redux-saga.

Take a look at this blog post: http://jaysoo.ca/2016/01/03/managing-processes-in-redux-using-sagas/

gaearon

gaearon commented on Jan 8, 2016

@gaearon
Contributor

Oh, in this case it's fine to keep it in component.
(Note: generally in React it's best to use declarative model for everything rather than showing modals imperatively with jQuery. But no big deal.)

catamphetamine

catamphetamine commented on Jan 8, 2016

@catamphetamine

@slorber Yeah, I'm already returning Promises and stuff from my "thunks" (or whatever you call them; errr, i don't like this weird word), so I can handle spinners and such.

@gaearon Ok, I got you. In the ideal machinery world it would of course be possible to stay inside the declarative programming model, but reality has its own demands, irrational, say, from the view of a machine. People are irrational beings not just operating with pure zeros and ones and it requires compromising the code beauty and purity in favour of being able to do some irrational stuff.

I'm satisfied with the support in this thread. My doubts seem to be resolved now.

gaearon

gaearon commented on Mar 18, 2016

@gaearon
Contributor

Relevant new discussion: #1528

juliandavidmr

juliandavidmr commented on Aug 27, 2016

@juliandavidmr

@gaearon very awesome explanation! 🏆 Thanks 👍

deleted a comment from 25308985 on Mar 23, 2018
locked and limited conversation to collaborators on Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @gregwebs@kevinold@vladar@merk@dariocravero

        Issue actions

          Trying to put API calls in the correct place · Issue #291 · reduxjs/redux