svrcekmichal / redux-axios-middleware

Redux middleware for fetching data with axios HTTP client
MIT License
918 stars 96 forks source link

The middleware now returns a new promise to allow for chaining. #9

Closed nmaves closed 8 years ago

nmaves commented 8 years ago

This should take care of issue #7.

nmaves commented 8 years ago

Any thoughts on this @svrcekmichal ?

svrcekmichal commented 8 years ago

@nmaves hi finally I have time to review, I've been quite busy lately.

  1. what's the argument for returning { payload, meta: { ...responseMeta }, action: newAction } or error value respectively? I believe returning just new action object is enough, because every one can create config to set what he need there
  2. If error is instance of Error, what should happened? There will be unfinished promise waiting for resolve in memory forever.
  3. result of client.request(...).then(...) is Promise, so why not use this one and create new? You can do return Promise.reject(newAction) in error handler or just return newAction in success handler and you will have the same functionality
nmaves commented 8 years ago
  1. I was just trying to remain consistent with what your middleware already was returning. I don't have a problem just returning the resolution action on success.
  2. Good catch, but I am not sure why we are checking for a type of Error. Do we really need this check?
  3. I am not sure what you are thinking here but maybe a code example might clear things up for me.
svrcekmichal commented 8 years ago
  1. To remain consistent, you should return newAction, because if you return { payload, meta: { ...responseMeta }, action: newAction } you are sending payload in .payload and .action.payload and you are sending all meta in .meta and .action.response. So it's all duplicate. Please, just return newAction object, this way everyone who will change onSuccess or onError handler in config will stay consistent with whatever he want
  2. It's from axios documentation of handling errors. Maybe we can use something, for me this was the fastest solution and maybe not so good. I will try to check how other middlewares handle errors. Maybe we can just send this error and make user resolve it later or just create new config options which will know how to handle this errors. User will be than able to pass function like this:
e => process.env.NODE_ENV !== 'production' ? console.log(e) : Raven.captureException(e) // for xample if project use sentry
  1. You don't need to create new promise
const promise = somePromise().then(function(response){
  //do whatever you want;
  return response;
}, function(error) {
  //do whatever you want;
  return Promise.reject(error);
})

If you do it this way, you can use promise.then(...) anywhere you want, you don't need to do:

return new Promise((resolve,reject)=>{
  const promise = somePromise().then(function(response){
    //do whatever you want;
    resolve(response);
  }, function(error) {
    //do whatever you want;
    reject(error);
  })
})
nmaves commented 8 years ago

I have updated per your suggestions. The only thing I was unsure about was the rejection with the fatal axios error. I don't like that we have two different objects being rejected.

svrcekmichal commented 8 years ago

I have created issue #10 . Please, if you have time make a little review, if you will be okay, I will create PR with that. I believe this solves the last issue. According to default action change, I would again suggest to return newAction from middleware, because it has information about current response and action which trigger this new action

nmaves commented 8 years ago

Michal, I have updated the middleware.js to reflect your suggestions. I will be away on holiday for the next week but I will check back in when I can.

svrcekmichal commented 8 years ago

@nmaves Enjoy your holiday. I have published package with version 1.0.0-rc.0 in case there's some bug ... you can check it out 👍