raineroviir / react-redux-socketio-chat

Full Stack Chat Application
942 stars 242 forks source link

Question: In UserAPIUtils, why is Promise.reject used sometimes and just reject others? #28

Closed AndrewBestbier closed 8 years ago

AndrewBestbier commented 8 years ago

This is more of a question for my understanding rather than an error. As seen in the UserAPIUtils.js file, Promise.reject is used as follows:

export function loadAuth() {
  return new Promise((resolve, reject) => {
    superagent
    .get('/api/load_auth_into_state')
    .end((err, res) => {
      if (err) {
        Promise.reject(err);
      } else {
        resolve(res.body);
      }
    });
  });
}

And other times not:

export function signOut() {
  return new Promise((resolve, reject) => {
    superagent
    .get('/api/signout')
    .end((err) => {
      if (err) {
        reject(err);
      } else {
        resolve(true);
      }
    });
  });
}

It does have different behaviours so I was wondering what the significance was?

raineroviir commented 8 years ago

Hi Andrew, great question. The promise.reject throws an error if there is an actual error or something like wrong credentials. It was a quick fix at the time since Mongoose/MobgoDB which checks the credentials are by callbacks and does not use promises. I hope I was helpful let me know if there's more questions On Tue, Dec 1, 2015 at 1:31 PM Andrew notifications@github.com wrote:

This is more of a question for my understanding rather than an error. As seen in the UserAPIUtils.js file, Promise.reject is used as follows:

export function loadAuth() { return new Promise((resolve, reject) => { superagent .get('/api/load_auth_into_state') .end((err, res) => { if (err) { Promise.reject(err); } else { resolve(res.body); } }); }); }

And other times not:

export function signOut() { return new Promise((resolve, reject) => { superagent .get('/api/signout') .end((err) => { if (err) { reject(err); } else { resolve(true); } }); }); }

It does have different behaviours so I was wondering what the significance was?

— Reply to this email directly or view it on GitHub https://github.com/raineroviir/react-redux-socketio-chat/issues/28.

AndrewBestbier commented 8 years ago

Hi raineroviir, thank you for your prompt response!

AndrewBestbier commented 8 years ago

Hi raineroviir, quick follow up question.

When one tries to sign in with incorrect credentials, the following errors are shown:

screen shot 2015-12-02 at 12 14 14 am

Now, in Redux the following constant should be dispatched to our reducer: 'AUTH_SIGNIN_FAIL'. However, that particular section in the reducer is never actually called. I've tried to console.log() there to debug and It never actually does anything. The code I am talking about is at:

case AUTH_SIGNIN_FAIL:
    console.log("failed"); //Never called
    return {
      ...state,
      signingIn: false,
      user: null,
      signInError: action.error
    };

However if you use reject(err) instead of Promise.reject(err), it is called. This has a knock-on effect though, when you try use the following code:

dispatch(Actions.signIn(userObj)).then(() => {
        console.log("test"); //Called for reject and resolve for resolve. Called only for resolve with Promise.resolve
        this.context.router.transitionTo('/chat');
      });

So my question is, which one to use? I want to use the failed post to our server to show user error handling on the client side through Redux. I can't seem to do this though with using the current reject(err) instead of Promise.reject(err) and also keeping the transitionTo('/chat') to not run when there is an error. Could you possibly point me in the right direction?

raineroviir commented 8 years ago

I'll take a look at this and see if I can figure out a solution, because it is a bit of problematic code. I think if we wrapped the result of our authentication logic with a promise that might do it. Because right now without that promise.reject in there, the invalid credentials error doesn't bubble up, so the promise resolves when it should fail, thus preventing the next .then call from firing

raineroviir commented 8 years ago

OK Well I would suggest you don't use this.context.router when switching routes and learn just a little bit about routing with react-router as I am about to right now. :)

AndrewBestbier commented 8 years ago

Yeah I've been reading quite a bit to try solve this particular problem. I implemented the solution found https://github.com/rackt/react-router/blob/master/docs/guides/advanced/NavigatingOutsideOfComponents.md

case AUTH_SIGNIN:
    history.replaceState(null, '/chat') //From the above mentioned link
    return {
      ...state,
      signingIn: true
    };

But then we navigate to the '/chat' room before the state has been set. This leads to a lot of missing properties.

raineroviir commented 8 years ago

Have you seen this library? I've heard good things and sounds like it may be capable of solving our problem: https://github.com/rackt/async-props

AndrewBestbier commented 8 years ago

Looks interesting, I'll check it out. In the meanwhile, I managed to get around the problem using: https://www.npmjs.com/package/whatwg-fetch.

Something like:

fetch('/example')
  .then(checkStatus)
  .then(parseJSON)
  .then(function(data) {
    dispatch(joinRoomSuccess(data));
     history.replaceState(null, '/chat')
  }).catch(function(error) {
    console.log('request failed', error)
  })

function joinRoomSuccess(data) {
  return {
    type: 'JOINING_ROOM_SUCCESS',
    data: data
  };
}

Obviously it adds up to a substantial amount of boilerplate, so I'm working on slimming it down

raineroviir commented 8 years ago

That flow looks good. Nice! I've been learning more about fetch recently and I'm using it on my latest project, it seems a whole lot better than superagent

AndrewBestbier commented 8 years ago

Cool :) I found it through the Redux documentation. Anyway I'll investigate cleaner ways of doing the above code and get back to you

raineroviir commented 8 years ago

I've moved over to a fork of whatwg-fetch: https://github.com/matthew-andrews/isomorphic-fetch. Really useful library and I no longer need to wrap network requests in Promises. Thanks!