processOptions.failType and dispatch in promise doesn't work

[kopax]

I am trying to throw an error in my promise chain with the following code :

  processOptions: { // options configuring the process hook below
    // on error/reject, automatically dispatch(repoLoadingError(err))
    failType: requestError, // action creator which accepts error
  },

  // perform async processing, Using redux-logic promise features
  // so it will dispatch the resolved action from the returned promise.
  // on error, it dispatches the using the action creator set in
  // processOptions.failType above `repoLoadingError(err)`
  // requestUtil was injected in store.js createLogicMiddleware
  process({ getState, authService, action }, dispatch) {
    const { username, password } = action.data;
    dispatch(sendingRequest(true));

    let codeUrl;
    return authService.preLogin()
      .then((res) => authService.login(username, password))
      .then((jwt) => {
        if (jwt) { // force the error
          return Promise.reject(new Error('It appear there is a problem with your account, please contact an administrator.'));
        }
        dispatch(put(jwtLoaded(jwt)));
      });

The reject doesn't get caught :

logic.js?d876:74 Uncaught (in promise) Error: It appear there is a problem with your account, please contact an administrator

This is the content of a authService.login :

  login(username, password) {
    const options = {
      method: 'POST',
      headers: {
        'Accept': 'application/json', // eslint-disable-line quote-props
        'Content-Type': 'application/x-www-form-urlencoded',
      },
    };

    const queryString = toQueryString({ username, password });

    return request(`${url.login}?${queryString}`, options)
      .then((response) => Promise.resolve(response));
  },

I have the feeling that it is not possibme to dispatch within the promise.
Is it a bug or is it me ?

[kopax]

Solved it using the example in #18

Be careful, even the examples from the documentation aren't up to date

[jeffbski]

@kopax Thanks for raising the issue. I believe that I need a nice usage guide that walks one through using the API and its variations. Maybe a cookbook too.

I believe the confusion lies in the fact that the API allows multiple signatures to accommodate different JS styles.

The full process signature is process(deps, dispatch, done) which allows for multiple dispatches.
However since it is common to need only a single dispatch from async processing, then omitting the done, switches to that mode. process(deps, dispatch)
And finally if you simply want to return obj, promise, or observable that resolve to actions then you don't need dispatch or done. process(deps)

I guess it would have been better to show the multiple dispatch example to avoid confusion.

I'm trying to consider how I can help people that are new to the API to get up to speed quickly. I'm thinking some guides and possibly some videos explaining things would help.

I'm not aware of any examples in the docs that are broken or not up to date, if you found any let me know and I'll update them.

[jeffbski]

In thinking more about this, I think that it would be safer for me to just show the multiple-dispatch signature since that is safer for people to get started with regardless of how they will use it. Then I can mention the other variations later.

I've started by updating the README and API doc to show using the multiple dispatch signature. I'll also go through and do the same with the examples.

[jeffbski]

Given the potential confusion with the single dispatch mode, maybe it should be eliminated altogether. I'll discuss this with the other users.

[kopax]

I still haven't find why my failType: requestError is ignored. I have to use the catch block my self.

Because I was using a finally block, it looks a bit ugly

dispatch(requestSending(true)
Promise.resolve(...)
   .then((res) => res)
   .catch((err) => dispatch(requestError(err)))
   .then((err) => dispatch(requestSending(false))

Do you have an idea ?

I've started by updating the README and API doc to show using the multiple dispatch signature. I'll also go through and do the same with the examples.

I think this is a good Idea, if you ask people to try it, the library should cover the most complicated case, including some unit testing example. So far, I can replace my sagas with logic but not my sagas test with logic tests

[jeffbski]

I believe I understand what you are running into here. If you are using the dispatch in your process signature then the return value is ignored. The error is being caught in the promise which is returned, but then it is ignored since you are using the dispatch.

If you were to not use dispatch and just return the promise then it would work like you expect, however then you don't have a way to do multiple dispatch (unless you returned an observable).

Typically I trigger my loading state off the original action so I don't need to do multiple dispatch and the same thing for turning off the loading state (I reset it on success or error). I just have my reducer that handles the state listen for all of those actions or if you are consistent then you could even use a regex.

But if you do want multiple dispatching then currently you can't mix using dispatch and the return value.

So dispatch like you are currently

dispatch(requestSending(true)
Promise.resolve(...)
   .then((res) => res)
   .catch((err) => dispatch(requestError(err)))
   .then((err) => dispatch(requestSending(false))

or using observables (here is one way to do this trying not to vary from your original code too much. There are many ways to do this)

process({ getState, action }) {
  return Observable.create(obs => {
    obs.next(requestSending(true));
    Promise.resolve(...)
      .then(res => obs.next(success(res)))
      .catch(err => obs.next(requestError(err)))
      .then(() => obs.next(requestSending(false)))
      .then(() => obs.complete());
  });
}

Thinking for ways we might extend the API we could consider allowing the return value to be used even if dispatch is used, but then people might have to be careful with their returning in their code. When using callbacks people often don't worry much about what is being returned. So it seems to make sense to only do one or the other (use the return value or rely on dispatching). Or I guess we could have a processOption that enables dispatching of the return even if dispatch is used?

If we want promise code to be able to dispatch multiple items, then maybe there should be a way to return an object or array of things to dispatch. We would either add another processOption or a helper function that wraps. Not sure, I am always trying to not overcomplicate things. Originally I was thinking that to keep things simple I would just have people use dispatch/done (or observables) when they need to do multiple dispatching.

I guess I need to clarify the docs for failType in some way so that people don't get caught like you did.

[kopax]

or using observables (here is one way to do this trying not to vary from your original code too much. There are many ways to do this)

This is the whole logic :

export const getAuthorizeLogic = createLogic({
  type: LOGIN_REQUEST, // trigger on this action
  cancelType: LOCATION_CHANGE, // cancel if route changes
  latest: true, // use response for the latest request when multiple
  processOptions: { // options configuring the process hook below
    // on error/reject, automatically dispatch(repoLoadingError(err))
    failType: requestError, // action creator which accepts error <==== does not work
    // successType: requestError,
  },
  process({ authService, action, forwardTo }, dispatch, done) {
    const { username, password } = action.data;
    dispatch(sendingRequest(true));
    let codeUrl;
    let token;
    return authService.preLogin()
      .then(authService.login(username, password))
      .then((res) => auth.code(oauthClient.clientId, oauthClient.redirectUri))
      .then((code) => {
        codeUrl = code.url;
      })
      .then(() => {
        // let's get the token
        const code = getParameter('code', codeUrl);

        if (!code) {
          throw new Error('It appear there is a problem with your account, please contact an administrator.');
        }
        return auth.token(oauthClient.clientId, oauthClient.clientSecret, code, oauthClient.redirectUri, oauthClient.scopes);
      })
      .then((jwt) => {
        if (!jwt) {
          throw new Error('It appear there is a problem with your account, please contact an administrator.');
        }
        return jwt;
      })
      .then((jwt) => {
        dispatch(sendingRequest(false));
        token = jwt;
      })
      .catch((err) => {
        dispatch(requestError(new ResponseMessage(err.message)))
        return true;
      })
      .then((err) => {
        if (err){
          forwardTo(pages.pageLogin.path); // Go to dashboard page
        } else {
          dispatch(jwtLoaded(token));
          dispatch(changeForm({ username: '', password: '' }));
          forwardTo(pages.pageDashboard.path); // Go to dashboard page
          done();
        }
      });
  },
});

// Bootstrap logic
export default [
  getAuthorizeLogic,
];

I am currently thinking this is not as good as my sagas for readability.
Would you split that in multiple logic ? It's hard to take a decision, how would you write it ?

[jeffbski]

There are many ways we could improve this. Here are a couple ideas:

1. async/await would help eliminate a bunch of the promise noise
2. refactor out main work of authenticating into a separate function so logic is only doing simple mapping to calls and dispatching.
3. I added a REQ_ERR_GOTO_LOGIN action so you could keep your original requestError action as only dealing with the error. When this action is used, it forwards to login and and then dispatches requestError.

Note that failType will catch thrown errors and dispatch them. The only reason it didn't work for you before is because your promises were catching them and returning them but since you were using dispatch the return values are not used (either dispatch or return, not both).

// forward to login page and dispatch requestError
export const requestErrorGotoLoginLogic = createLogic({
  type: REQ_ERR_GOTO_LOGIN,
  process({ action, forwardTo }) {
    forwardTo(pages.pageLogin.path); // Go to login page 
    return requestError(action.payload); // return err action to dispatch
  }
});


export const getAuthorizeLogic = createLogic({
  type: LOGIN_REQUEST, // trigger on this action
  cancelType: LOCATION_CHANGE, // cancel if route changes
  latest: true, // use response for the latest request when multiple
  processOptions: {
    failType: REQ_ERR_GOTO_LOGIN // action creator for errors thrown
  },
  
  async process({ authService, action, forwardTo }, dispatch, done) {
    dispatch(sendingRequest(true));
    const { username, password } = action.data;
    const jwt = await performLogin(username, password);
    dispatch(sendingRequest(false));
    dispatch(jwtLoaded(jwt));
    dispatch(changeForm({ username: '', password: '' }));
    forwardTo(pages.pageDashboard.path); // Go to dashboard page
    done();
  },
});

async function performLogin(username, password) {
      const preLoginRes = await authService.preLogin();
      const res = await authService.login(username, password)(preLoginRes);
      const codeRes = await auth.code(oauthClient.clientId, oauthClient.redirectUri);
      const codeUrl = codeRes.url;
      const code = getParameter('code', codeUrl);
      if (!code) {
        throw new Error('It appear there is a problem with your account, please contact an administrator.');
      }
      const jwt = await auth.token(oauthClient.clientId, oauthClient.clientSecret, code, oauthClient.redirectUri, oauthClient.scopes);
      if (!jwt) {
          throw new Error('It appear there is a problem with your account, please contact an administrator.');
      }
      return jwt;
}

If you would prefer to not have the extra REQ_ERR_GOTO_LOGIN action then you could instead use a try catch and not apply the failType option. So instead it would look like

export const getAuthorizeLogic = createLogic({
  type: LOGIN_REQUEST, // trigger on this action
  cancelType: LOCATION_CHANGE, // cancel if route changes
  latest: true, // use response for the latest request when multiple
 
  async process({ authService, action, forwardTo }, dispatch, done) {
    dispatch(sendingRequest(true));
    const { username, password } = action.data;
    try {
      const jwt = await performLogin(username, password);
      dispatch(sendingRequest(false));
      dispatch(jwtLoaded(jwt));
      dispatch(changeForm({ username: '', password: '' }));
      forwardTo(pages.pageDashboard.path); // Go to dashboard page
    }  catch (err) {
          dispatch(sendingRequest(false));
          forwardTo(pages.pageLogin.path); // Go to dashboard page
    }
    done();
  },
});

Even if you don't want to use async/await then you could still use promises just pull that code into a function like I did to get it out of the logic, it doesn't need to live all in there.

You could alternatively use generators to cleanup the promise noise too. I think async/await is pretty compelling though.

[kopax]

Ok, I will follow your suggestion, it doesn't sounds too hard, and your prefer this for a reason.

Just a question, how does the payload has been passed to

export const requestErrorGotoLoginLogic = createLogic({
  type: REQ_ERR_GOTO_LOGIN,
  process({ action, forwardTo }) {
    forwardTo(pages.pageLogin.path); // Go to login page 
    return requestError(action.payload); // <=== how do we got this action dispatched with an object containing payload ?
  }
});

Also, should this function be exported like this :

// Bootstrap logic
export default [
   requestErrorGotoLoginLogic,
  getAuthorizeLogic,
];

Because I don't see any cancelType for it.

I have also tried to add a console.log in the requestErrorGotoLoginLogic function to see the action content but it never get to there, like if the cancelType was ignored.

Unit testing

I would also be very curious on how would look like the unit test for such a logic.

I have my old saga test, will this help ?

describe('loginFlow Saga', () => {
  let getLoginWatcherGenerator;

  // We have to test twice, once for a successful load and once for an unsuccessful one
  // so we do all the stuff that happens beforehand automatically in the beforeEach
  beforeEach(() => {
    getLoginWatcherGenerator = getLoginWatcher();

    const selectDescriptor = getLoginWatcherGenerator.next().value;
    expect(selectDescriptor).toEqual(select(selectUsername()));

    const requestURL = `https://api.github.com/users/${username}/repos?type=all&sort=updated`;
    const callDescriptor = getLoginWatcherGenerator.next(username).value;
    expect(callDescriptor).toEqual(call(request, requestURL));
  });

  it('should dispatch the reposLoaded action if it requests the data successfully', () => {
    const response = [{
      name: 'First repo',
    }, {
      name: 'Second repo',
    }];
    const putDescriptor = getLoginWatcherGenerator.next(response).value;
    expect(putDescriptor).toEqual(put(reposLoaded(response, username)));
  });

  it('should call the repoLoadingError action if the response errors', () => {
    const response = new Error('Some error');
    const putDescriptor = getLoginWatcherGenerator.throw(response).value;
    expect(putDescriptor).toEqual(put(repoLoadingError(response)));
  });
});

describe('getLoginWatcher Saga', () => {
  const getLoginWatcherSaga = getLoginWatcher();

  it('should watch for LOGIN_REQUEST action', () => {
    const takeDescriptor = getLoginWatcherSaga.next().value;
    expect(takeDescriptor).toEqual(fork(takeLatest, LOGIN_REQUEST, getLogin));
  });
});

describe('loginData Saga', () => {
  const loginDataSaga = loginData();

  let forkDescriptor;

  it('should asyncronously fork getLoginWatcher saga', () => {
    forkDescriptor = loginDataSaga.next();
    expect(forkDescriptor.value).toEqual(fork(getLoginWatcher));
  });
  // NOTE: disable due to error: "Generator is already running", see: https://github.com/mxstbr/react-boilerplate/issues/1281
  it('should yield until LOCATION_CHANGE action', () => {
    const takeDescriptor = loginDataSaga.next();
    expect(takeDescriptor.value).toEqual(take(LOCATION_CHANGE));
  });
  
  it('should finally cancel() the forked getLoginWatcher saga',
    function* loginDataCancellable() {
      // reuse open fork for more integrated approach
      forkDescriptor = loginDataSaga.next(put(LOCATION_CHANGE));
      expect(forkDescriptor.value).toEqual(cancel(forkDescriptor));
    }
  );
});

One good point with redux-saga was that unit testing was pretty easy, calling the .next from the generator to get the next yield value. Probably with async and await there are similar mathod. I am still not familiar with Observable.

What is the easiest way to write test for our logic ?

I have tried to write something but the problem is I don't see how to test :

• async
• await
• preconfigure logic and helpers
• trigger logic

[jeffbski]

In my example since I had set processOptions.failType to REQ_ERR_GOTO_LOGIN it will take any thrown error and use this to create the error action. If failType is a string, then it uses it as the action type and following flux standard action format it creates:
{ type: REQ_ERR_GOTO_LOGIN, payload: err, error: true }

If the failType is an action creator, it will call it passing in the err and then you can format the action however you like.

Yes, you would include all of your logic in the export default array so that it gets loaded. The inline exports are really only used for testing so all logic needs to be provided in the export default array to get run in the store.

For much of my testing, I usually just focus on the key parts, so I typically test the validate, transform, and process hooks if I implemented them.

So I import the logic and call the hooks directly passing in data or mocks to test.

For instance, if I had this logic:

import { createLogic } from 'redux-logic';
import { LOCATION_CHANGE } from 'react-router-redux';
import { LOAD_REPOS } from 'containers/App/constants';
import { reposLoaded, repoLoadingError } from 'containers/App/actions';
import { selectUsername } from 'containers/HomePage/selectors';

/**
 * Github repos request/response handler
 */
export const getReposLogic = createLogic({
  type: LOAD_REPOS, // trigger on this action
  cancelType: LOCATION_CHANGE, // cancel if route changes
  latest: true, // use response for the latest request when multiple

  processOptions: { // options configuring the process hook below
    // on error/reject, automatically dispatch(repoLoadingError(err))
    failType: repoLoadingError, // action creator which accepts error
  },

  // perform async processing, Using redux-logic promise features
  // so it will dispatch the resolved action from the returned promise.
  // on error, it dispatches the using the action creator set in
  // processOptions.failType above `repoLoadingError(err)`
  // requestUtil was injected in store.js createLogicMiddleware
  process({ getState, requestUtil /* , action */ }) {
    const username = selectUsername()(getState());
    const requestURL = `https://api.github.com/users/${username}/repos?type=all&sort=updated`;
    return requestUtil(requestURL) // return a promise resolving to action
      .then((repos) => reposLoaded(repos, username)); // resolve w/action
  },
});


// Bootstrap logic
export default [
  getReposLogic,
];

then I could test it like

import expect from 'expect';
import { LOCATION_CHANGE } from 'react-router-redux';

import { getReposLogic } from '../logic';

import { LOAD_REPOS,
         LOAD_REPOS_SUCCESS,
         LOAD_REPOS_ERROR } from 'containers/App/constants';
import { reposLoaded /* , repoLoadingError */ } from 'containers/App/actions';

import { createLogicMiddleware } from 'redux-logic';
import Imm from 'immutable';

describe('getReposLogic', () => {
  const username = 'jeffbski';
  const getState = () => Imm.fromJS({
    home: {
      username,
    },
  });

  it('should make request and dispatch the reposLoaded action once data returns', () => {
    const repos = [
      { id: 1, name: 'foo' },
      { id: 2, name: 'bar' },
    ];
    function requestUtil(url) { // mock requestUtil so elim network
      expect(url).toBe(`https://api.github.com/users/${username}/repos?type=all&sort=updated`);
      return new Promise((resolve) => resolve(repos));
    }
    const resultPromise = getReposLogic.process({ getState, requestUtil });
    const expectedResultAction = reposLoaded(repos, username);
    return resultPromise
      .then((action) => {
        expect(action).toEqual(expectedResultAction);
      });
  });

  it('should reject with an error if the request fails', () => {
    function requestUtil(/* url */) { // mock requestUtil so elim network
      return new Promise((resolve, reject) => reject(new Error('Not Found')));
    }
    const resultPromise = getReposLogic.process({ getState, requestUtil });
    return resultPromise
      .then(() => { throw new Error('should not be successful'); },
            (err) => {
              expect(err.message).toBe('Not Found');
            });
  });
});

But then I do include some integration tests which test not only the hooks but that everything is configured properly with the correct actions, triggers, async operation, cancellation, and all.

For those I typically use createLogicMiddleware and a few spies. You can see an example here

You can also obviously use full createStore applyMiddleware etc to do integration testing too, but it's usually easier to be a little lower level where you can see everything that is going on all the next and dispatch calls besides the data changes.

I am actually considering creating another module that will further simplify this lower level integration testing so you don't have to create spies and all that, it could just set things up for you to make it easier to get going.

[jeffbski]

Maybe we should split discussions that are in different topics into separate issues. We are all over the place in this one. If you are ok with the answers for the original topic, how about we close this one and we can create new ones for continued testing or other discussion?

[kopax]

I am actually considering creating another module that will further simplify this lower level integration testing so you don't have to create spies and all that, it could just set things up for you to make it easier to get going.

I like this idea.

How about we close this one and we can create new ones for continued testing or other discussion?

you are right, I am closing this one, work a bit on what you gave me and if I have further questions I'll create another thread.