Never rejecting promise in promise middleware

[klis87]

Just an idea, maybe promise could be always resolved, but with an object with one of the following params: success, error, abort

Why? Because otherwise there are issues with requests aborts which will raise unhandling promise rejection error. The same goes for error, sometimes you don't want to handle an error in a specific place because there aren't any predictable error and random network errors you wanna handle globally.

After this, we could give up meta.asPromise as well as auto config in promise middleware . We could promisify all requests simply because there would be no negative side of doing so, it would reduce the boilerplace.

Connected with #93 , #128 , #179 , @nemosupremo , @mowbell, @megawac , what do you think?

[klis87]

@strdr4605 If you have some time, I would like to know your opinion. On removing saga branch actually I will remove also promise middleware. This does not make sense anymore as because request sending logic will be in another middleware which will replace sagas, I can return promise there directly, simplifying the code.

And also because saga will be removed, promisified request action will be the only way to wait for a response in the place you dispatch request action, which is often needed.

Previously this could also be done with sendRequest which must be gone as it is just saga.

I want to remove meta.asPromise, handleRequest promisify and autoPromisify options.

The reason for those to exist was to avoid unhandled promise rejection warning...

The problem is, if you distatch(fetchSth()), this returns promise which will be resolved on success and rejected on error and abort. But often you dont care about it, you can for instance handle exception in another place, or even you might not doing this at all as error is kept inside reducer.

I am thinking the best would be to always resolve promise with either success, error or abort key. Then no warning will be present. Also maybe it is more logical as we have 3 potential response types and promise supports only 2.

What do you think?

[strdr4605]

I like more the idea of getting the request results from the redux state and not from dispatch(fetchSth()).then(.... This is why I like using redux middleware like redux-saga.
About redux-saga-requests I would also like to use it as a middleware (even without redux-saga) and access data only through redux state.
Can you give an example where I would need to use dispatch(fetchSth()).then(... instead of getting data from the state.

I don't really want to promisify every action by default. And I think that error and abort should be dispatched on Promise.reject as it's a natural way.
Never rejecting promise may confuse people who will have some issues and try to debug. The same applies to new contributors.

This library already enforces some constraints which are hard to understand and adopt for some people (as in the case with my team, and even I have confusion).

To still solve this case where dispatch(fetchSth()).then(... is needed I would suggest having a custom
dispatch function that will promisify the action. Something like this:

import React from 'react'
import { useResolveDispatch } from 'react-saga-requests'

export const Component = ({ state }) => {
  const resolveDispatch = useResolveDispatch()

  resolveDispatch(fetchBooks()).then(successOrErrorOrAbortAction => {
  // do something with action
  })

  return (
    <div>
      ...
    </div>
  )

[klis87]

@strdr4605

I like more the idea of getting the request results from the redux state and not from dispatch(fetchSth()).then

me to, and usually this is what I do, but sometimes it is more convenient to handle response in the place you dispatch request action, for example:

1. In React component in onSubmit like:

onSubmit = values  => {
  dispatch(doMutation(values)).then(resetForm);
}

This is a very common use case especially to update some local state not kept in Redux. As a comparison, this is very easy to do with redux-thunk which returns promise, but hard or impossible with redux-saga - you would need to pass resolve/reject to saga or resetForm to call inside saga, which is terrible as you then pass ui related logic to redux layer

2. Some people like to do SSR by implementing static methods in top Route component, for instance you have a component with method:

static fetch() {
  return store.dispatch(fetchSth())
}

Then they collect all components, call all static fetch methods and wrap in Promise.all

3. Using saga u can easily do sth like yield take(success(FETCH_STH)), but some people don't like it, I had issues in this library where people said they prefer to get response in the place they dispatch request, so for example they could do const response = yield putResolve(fetchSth())

There are many more use cases like that, those are just examples.

I don't really want to promisify every action by default.

I am thinking about doing this so you don't need to worry about it, generally promisyfying request action doesn't cause any problems, if you do dispatch(fetchSth()) and you don't care about result, then you don't care it is promise or not

The only disadvantage is this promise rejection warning... that's why I was thinking about having:

const { response, error } = await dispatch(fetchSth());

// not
try {
  const response = await(fetchSth());
} catch(e) {
  
}

I just want to weight all pros and cons and decide, whether allow not to promisify all actions and whether resolve or reject errors and aborts. I favour promisyfying everything because I could remove some options from API and resolving always to avoid unhandle rejection warnings. But maybe I am missing some cons of doing so, that's why I created this topic.

This library already enforces some constraints which are hard to understand and adopt for some people (as in the case with my team, and even I have confusion).

Could you please give me examples? I really want this library to be easy to use and logical. It is a good time to improve things before 1.0, but I need to know what problems are. Whether this is confusing API or missing examples/docs.

useResolveDispatch

Interesting idea, it could be done theoretically by wrappping useDispatch and for instance adding special action.meta. This is only for React though, many people would still want to have this in redux layer, for examples in sagas like I showed in examples

I really admit this topic is a tough one and I really want to make a good call before 1.0.

[strdr4605]

As I see usage for Never rejecting promise

onSubmit = async (values)  => {
  const { success } = await dispatch(doMutation(values));
  if (success) {
    resetForm(success.data);
  }
}

onSubmit = async (values)  => {
  dispatch(doMutation(values))
    .then(response => {
        if (response.success) {
          resetForm(response.success.data);
        }
    });
}

instead of

onSubmit = async (values)  => {
  try {
    const response = await dispatch(doMutation(values));
    resetForm(response.data);
  } catch() {}
}

onSubmit = async (values)  => {
  dispatch(doMutation(values))
    .then(response => {
          resetForm(response.data);
    }).catch();
}

So the question is:
another if or try ... catch?

---

2 of my coworkers agreed that “never rejecting promise” may make sense.

[klis87]

Thx for the feedback. For now I favour never rejecting promise, because unhandled promise rejections cannot really be avoided for aborts (which is not even an error) or errors if u just handle them in other places, like by reading from state. This not only spams console, sentry if u use it, but also could exit nodejs process with ssr, I am not sure for which node version, but I read in the past that this warning will actually exit nodejs process in future nodejs version.

Anyway, assuming we will go this way, I would actually flatten this a little, so:

1. for success I would resolve as { data, action: successAction }
2. for error I would resolve as { error, action: errorAction },
3. for abort I would resolve as { aborted: true, action: abortAction }

So as you see, no response but data directly, which thogether with error will resemble useQuery hook, which is good I think.

Also, in all cases I would pass action key with response action (no matter what type) if someone needs it. One could even access request action as action.meta.requestAction.

Also, in theory I could make it customizable, but really I dont want to do it as I want to keep API to the minimum, which also avoids confusion.

One could always add own middleware to listen for promises which could once again reject them for abort and error responses.

[strdr4605]

So interface will look like:

interface PromiseAction<DataType = any> {
  data: null | DataType[] | DataType;
  error: null | Error;
  aborted: boolean;
  action: SuccessAction | ErrorAction | AbortAction;
}

and

const successResult = await dispatch(fetchSth()); 
// { data: {}, error: null, aborted: false, action: {}}

const errorResult = await dispatch(fetchSth()); 
// { data: null, error: Error, aborted: false, action: {}}

const abortResult = await dispatch(fetchSth()); 
// { data: null, error: null, aborted: true, action: {}}

I would also suggest changing aborted -> isAborted.

[klis87]

Yeah, that would be it, just I think that data would be data: null | DataType;, this array was for batch request I guess, but this can be any type (because interceptor or meta.getData could change it like https://github.com/klis87/redux-saga-requests/blob/master/examples/basic/src/store/actions.js#L12), and if it is array, one could pass DataType as array directly.

I would also suggest changing aborted -> isAborted.

Thx

[strdr4605]

data: null | DataType[] | DataType;

I added array because of https://github.com/klis87/redux-saga-requests#getquery:

multiple: set to true if you prefer data to be [] instead of null if data is empty, false by default

[klis87]

Good eye, and actually in previous version it might be like that.

But in current version multiple is only on selector level, and dispatch should resolve data just to those returned from server, plus optionally modified with onResponse interceptor and action.meta.getData

[strdr4605]

So

const response = await dispatch(fetchSth());

will have data that comes from the server and NOT data that comes from the server AND was processed with action.meta.getData?

[klis87]

@strdr4605 both, the procedure is this:

1. middleware awaits response, for error it just returns error, for success see data processors in next points
2. if batch request, mutliple responses are merged as one object with data as array, for normal requests nothing is done
3. meta.getData is called if defined
4. interceptor onResponse (I should rename it to onSuccess) is called

and only that processed data is then passed to reducer and returned from promise dispatch

also, if you have ssr response or cached response, those 2-4 of course are not done, as it was already done for initial request. This is because caching works like that, if you fire request and sth is cached, then instead of making request it just gets state from reducer and returns that, but that was already processed with getData etc so it won't be again

[klis87]

Done in 0.28.0