Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promise middleware force you to catch and then all requests #128

Closed
mowbell opened this issue Jun 15, 2018 · 7 comments
Closed

Promise middleware force you to catch and then all requests #128

mowbell opened this issue Jun 15, 2018 · 7 comments

Comments

@mowbell
Copy link

mowbell commented Jun 15, 2018

Can I mix promise middleware only for some requests and normal requests without handle these as promises?
When I use optional promise middleware, I must handle all requests (then and catch)
What if I need handle only ones as promises, and the rest as normal redux-saga-requests?

For fix it on my project, I override requestsPromiseMiddleware config as:

requestsPromiseMiddleware({
  getRequestAction: (action) => {
    return action.meta && action.meta.requestAction && action.meta.requestAction.isPromise ? action.meta.requestAction : null;
  },
})

Then I write actions creators that needs promise handling as:

export const sendContactMessage = ({ name, email, message }) => ({
  type: SEND_CONTACT_MESSAGE,
  isPromise:true,
  request: {
    url: '/contact',
    method: 'post',
    data: {
      name,
      email,
      message,
    },
  },
});

Maybe someone with the same problem can do it, but
My question, It is an issue or How can I do the same?

@mowbell mowbell changed the title Promise middleware force you to catch and then all request Promise middleware force you to catch and then all requests Jun 15, 2018
@klis87
Copy link
Owner

klis87 commented Jun 15, 2018

@mowbell Good point. Could you please show me an example of dispatching an action which you dont try catch and what are the consequences? Is it in saga or in a React component? I am asking because I wasnt bit by it apart from #93, but this only gives me a warning in the console without any consequences, otherwise everything works.

Adjusting getRequestAction this way is not a good idea, because getRequestAction is for indentifying request response actions actually (naming is indeed a little unfortunate and I need to improve it). With what you did you end up with all requests promises which will never get resolved.

What we can do is to add possibility to configure both request and response actions in middleware, or we could adjust promise middleware to look at action's meta.isPromise flag, so this could be configurable in the action itself, like you suggested. I think the latter is a better, simpler and more scalable approach.

What do you think?

@klis87
Copy link
Owner

klis87 commented Jun 15, 2018

Regarding getRequestAction, this actually should be named like getRequestActionFromResponse - it is used to match requests to responses by request action reference.

@mowbell
Copy link
Author

mowbell commented Jun 15, 2018

Hi @klis87
Yes, I agree with you, It's better configure a flag in the meta instead of add it in the action as plain param, In this way not affect FSA style actions creators implementations

Responding to your question, for me works my previous suggested implementation
As long as I write this:

...
requestsPromiseMiddleware({
  getRequestAction: (action) => {
    return action.meta && action.meta.requestAction && action.meta.requestAction.isPromise ? action.meta.requestAction : null;
  },
})
...

In my code (It's an implementation for React Native), the complete Redux setup is similar to:

import { createStore, applyMiddleware } from 'redux';
import createSagaMiddleware from 'redux-saga';
import { requestsPromiseMiddleware } from 'redux-saga-requests';
import axios from 'axios';
import reducer from '../reducers';
import rootSaga from './sagas';

const sagaMiddleware = createSagaMiddleware();
const sagaRequestsPromiseMiddleware = requestsPromiseMiddleware({
  getRequestAction: action => (action.meta && action.meta.requestAction && action.meta.requestAction.isPromise ? action.meta.requestAction : null),
});
const middlewares = [sagaMiddleware, sagaRequestsPromiseMiddleware];
const storeEnhancer = applyMiddleware(...middlewares);


const store = createStore(reducer, storeEnhancer);
const axiosInstance = axios.create({
  baseURL: 'https://URL',
  headers: { Accept: 'application/json', 'Content-Type': 'application/json' },
});
sagaMiddleware.run(rootSaga, axiosInstance);
export default store;

The example of dispatching an action (as non-promise) is normal as it appears in documentation:
(I don't use FSA)

export const sendContactMessage = ({ name, email, message }) => ({
  type: SEND_CONTACT_MESSAGE,
  name,
  email,
  message,
  request: {
    url: '/contact',
    method: 'post',
    data: {
      name,
      email,
      message,
    },
  },
});

I dispatch those non-promise actions from React component as:

<ProfileScreenView
        userData={userData}
        onSendContact={this.props.sendContactMessage}
        contactRequestStatus={contactRequestStatus}
      />

ProfileScreenView prop sendContactMessage is connect to redux with bindActionCreators

And, for request that needs to be handled as promises, I dispatch the actions as follow:
(with flag isPromise)

export const sendFeedbackService = (serviceId, problemType = ProblemTypes.NONE) => ({
  type: REQUEST_SEND_FEEDBACK_SERVICE,
  serviceId,
  problemType,
  
  isPromise: true,
  request: {
    url: `/services/${serviceId}/feedback`,
    method: 'put',
    data: {
      conflictType: problemType,
    },
    meta: {
      // meta is optional, it will be added to success, error or abort action when defined
      serviceId,
    },
  },
});

I dispatch those promise actions from React component as:

handleRequestFeedbackService: ({ sendFeedbackService, service, setRequestFeedbackStatus }) => (problemType) => {
        clearTimmer();
        setRequestFeedbackStatus(Statuses.SENDING);
        const problemTypeID = invert(ProblemTypes)[problemType];
        sendFeedbackService(service.serviceId, problemTypeID, problemDescription, rating)
          .then(() => {
            setRequestFeedbackStatus(Statuses.SUCCESS);
          })
          .catch(() => {
            setRequestFeedbackStatus(Statuses.FAILED);
            clearTimmer();
            timer = setTimeout(() => {
              setRequestFeedbackStatus(Statuses.NONE);
              clearTimmer();
            }, 2500);
          });
      },

@mowbell
Copy link
Author

mowbell commented Jun 15, 2018

Regarding warning, @klis87
For me in React Native, the non-promises actions doesn't respond and throws me error

So, as you suggest, a isPromise(or similar) flag should be added in meta

Then, getRequestAction (or if is changed to getRequestActionFromResponse ),
It could be like this:

getRequestAction: action => (action.meta && action.meta.requestAction && action.meta.requestAction.meta && action.meta.requestAction.meta.isPromise ? action.meta.requestAction : null),

Then, We can dispatch actions as:

export const sendFeedbackService = (serviceId, problemType = ProblemTypes.NONE) => ({
  type: REQUEST_SEND_FEEDBACK_SERVICE,
  serviceId,
  problemType,
  
  
  request: {
    url: `/services/${serviceId}/feedback`,
    method: 'put',
    data: {
      conflictType: problemType,
    },
    meta: {
      // meta is optional, it will be added to success, error or abort action when defined
      serviceId,
      isPromise: true,
    },
  },
});

@klis87
Copy link
Owner

klis87 commented Jun 16, 2018

Thanks for the code snippets.

Your adjustment getRequestAction indeed works but it makes all your request promise pending forever, which is not too optimal :)

Btw, in action below:

export const sendContactMessage = ({ name, email, message }) => ({
  type: SEND_CONTACT_MESSAGE,
  name,
  email,
  message,
  request: {
    url: '/contact',
    method: 'post',
    data: {
      name,
      email,
      message,
    },
  },
});

you added name, email, message to top action params because I guess u need those values in reducers and/or sagas? If thats is so, just small tip, consider adding those to meta, then those attrs will be also available in meta in success, error and abort actions, otherwise you would need to get it in success action for instance like action.meta.requestsReducer.param instead of just action.meta.param

Thats why our updated getRequestAction implementation could look like:

getRequestAction: action => action.meta && action.meta.isPromise ? action.meta.requestAction : null)

but I guess your solution might be a safer option due to potential meta.isPromise collision.

I will try to add this to the library in upcoming days.

@klis87
Copy link
Owner

klis87 commented Jul 8, 2018

@mowbell Published as 0.14.0, pls let me know if sth doesn't work as expected.

@mowbell
Copy link
Author

mowbell commented Jul 10, 2018

@klis87 Thanks a lot, it's working as expected, Great job!
I have been updating my code and everything has gone well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants