lelandrichardson / redux-pack

Sensible promise handling and middleware for redux
MIT License
1.33k stars 60 forks source link

question: side-effects and dispatch #58

Open tony-kerz opened 7 years ago

tony-kerz commented 7 years ago

in this section of the doc on side-effects, it shows something like:

import { sendAnalytics } from '../analytics';
import { doFoo } from '../api/foo';

export function userDoesFoo() {
  return {
    type: DO_FOO,
    promise: doFoo(),
    meta: {
      onSuccess: (result, getState) => {
        const userId = getState().currentUser.id;
        const fooId = result.id;
        sendAnalytics('USER_DID_FOO', {
          userId,
          fooId,
        });
      }
    }
  }
}

in the above sample, sendAnalytics looks a lot like an action creator (with type and payload as args), but submitting actions to redux requires a reference to dispatch somewhere.

is the implication that sendAnalytics submits a redux action, and if so, what is the implied strategy for obtaining a reference to dispatch?

or maybe, i'm asking the wrong question, but my general question is:

what would be the idiomatic way to manage a side-effect with redux-pack that wants to submit another redux action?

or is that an anti-pattern that redux-pack is specifically attempting to avoid?

apologies for being dense, i admittedly struggle with extracting "best practices" for side-effects from related discussions...

my immediate use-case is: on the failure of a data-fetch action i want to submit a subsequent ui-centric action concerned with managing errors (e.g. notify the user visually of an error)

tony-kerz commented 7 years ago

i subsequently stumbled across this comment on topic which shows a way to use redux-thunk to get a reference to dispatch, and i cobbled together a pattern like the following:

      function doSomething() {
        return dispatch => {
          dispatch({
            type: SOMETHING,
            promise: somethingPromise(),
            meta: {
              onSuccess: result => {
                dispatch({
                  type: SOMETHING_SUCCEEDED, 
                  promise: somethingSucceededPromise()
                })
              },
              onFailure: result => {
                dispatch({
                  type: SOMETHING_FAILED, 
                  promise: somethingFailedPromise()
                })
              }
            }
          })
        }
      }

wondering where this might fall on the spectrum of anti-pattern <-> idiomatic...

icopp commented 7 years ago

I've got the same general line of thought in mind. Just like the last time I commented on this project, having dispatch available in meta seems like it would give tremendous flexibility to replace redux-thunk with something more organized.

duro commented 7 years ago

I too am curious whether this is an anti-pattern? We have a need to do something like this, and if it is indeed NOT some kind of massive anti-pattern, I agree with @icopp that it should be integrated into the meta callbacks.

duro commented 7 years ago

@icopp and @tony-kerz:

Based on the following in this projects README, I have a feeling the author considers this an anti-pattern:

Async actions in redux are often done using redux-thunk or other middlewares. The problem with this approach is that it makes it too easy to use dispatch sequentially, and dispatch multiple "actions" as the result of the same interaction/event, where they probably should have just been a single action dispatch.

This can be problematic because we are treating several dispatches as all part of a single transaction, but in reality, each dispatch causes a separate rerender of the entire component tree, where we not only pay a huge performance penalty, but also risk the redux store being in an inconsistent state.

redux-pack helps prevent us from making these mistakes, as it doesn't give us the power of a dispatch function, but allows us to do all of the things we were doing before.

Given this encouragement, I was able to take a step back and look at our case where we thought a dispatch side-effect was needed, and see how it could be combined into a single action.

icopp commented 7 years ago

In my case, I want these dispatches to render separately, because I have multiple asynchronous actions that can trigger each other but that all have different UI side effects.

For example: pushing a button triggers action A with promise A, which alters things in the store initially, on a successful resolution, and on a failure (optimistic update style, re-rendering things accordingly for each case), but once it's successfully resolved (but not if it fails) I have to also trigger action B with promise B, which on a successful resolution alters other things in the store based on side effects of A on the server (and also re-renders things accordingly).

tony-kerz commented 7 years ago

not sure if this is what @icopp is alluding to, but in my case, i want to dispatch something conditionally based on the success or failure of the initial action/promise, so i don't think combining actions works for that use-case...?

stephanebachelier commented 7 years ago

Have you thought about using epics via redux-observable or saga via redux-saga for these side effects ? I've found using epics is way more powerful than using any kind of sequential / meta actions.

EnshaednHiker commented 6 years ago

My take on the redux-pack way of doing something is this: your actions should not be split up into various actions based upon the possible lifecycles of an action. So the developer has the space, I think, to decide what an entire action looks like, what looks sensible as the entire lifecycle of an action and go from there. If one action triggers another, I think it makes sense to trigger that action in the meta part of a redux-pact action. An easy way do that is with redux-thunk. Redux-thunk gives you access to the store and dispatch an action from anywhere in your app. So, for instance, you can in your actions.js:

import store from '~/public/store.js';

and then in an action do this:

export const REGISTER = 'REGISTER';
export function register(payload){
    return {
        type: REGISTER,
        promise: system.API.POST('/users',{"payload":payload}),
        meta: {
            onSuccess: (result, getState) => {
                if (result.status===201){
                    store.dispatch(login(payload));
                }
            },
            onFailure: (err, getState) => {
                console.warn(err);
                store.dispatch(clearCredentials());
            }
        }
    }
}

Although the author might consider this an anti-pattern, it feels right to me for its context in my app, and works slick.

YaaMe commented 6 years ago

Why not move it to middleware? like that

export const analyticsMiddleware = store => next => action => {  
    switch (action.type) {
        case DO_FOO: 
            const { meta } = action;
            const lifecycle = meta ? meta[KEY.LIFECYCLE] : null;

            switch (lifecycle) {
                case LIFECYCLE.SUCCESS: store.dispatch(SOMETHING_SUCCEEDED); break;
                case LIFECYCLE.FAILURE: store.dispatch(SOMETHING_FAILED); break;
            }
        default: ;
    }
    return next(action);
}

:P