salsita / prism

React / Redux action composition made simple http://salsita.github.io/prism/
496 stars 24 forks source link

Redux-saga side effects after unmounting component #39

Closed jmarceli closed 8 years ago

jmarceli commented 8 years ago

Conclusion:

Sagas are canceled by redux-elm Updater after unmounting a component

----- ORIGINAL question I've got a problem which might not be related to redux-elm but I'm not sure so I'll ask.

Code in question looks like that:

function* fetchData() {
  yield put({ type: 'DataLoading' });
  const result = yield call(Effects.getOrders);
  // this line is never executed if component gets unmounted before API returns some results
  yield put({ type: 'DataLoaded', result: result.data });
}

It's responsible for handling API calls and works pretty well as long as I'm waiting for DataLoaded event. If I change active page (with react-router) before data gets loaded then DataLoaded event never fires. It looks like the app doesn't return to the generator function fetchData. I'm confused and have no idea how to debug this one. Any hint or tip would be much appreciated.

eliperelman commented 8 years ago

Is that DataLoading specific to the view you are on? Do you see anything in the console about the saga being cancelled?

tomkis commented 8 years ago

That's actually a design decision we made. We had to tie Saga with component lifecycle and it's desired to cancel any outgoing Sagas upon component unmounting.

And frankly I believe that canceling the API call when leaving the Component is great because you don't need to worry about "phantom" API responses.

But I assume your issue is that you set loading spinner upon DataLoading but you will never reset it, right?

jmarceli commented 8 years ago

Yeah that's my issue. When I come back to the component it is still in "loading" state (with spinner icon). There are two possible solutions I can think of:

  1. Handle all the API calls by separate component which will never be unmounted by react-router.
  2. Reset loading state if mounted component is in loading state but don't do anything if component is in "loaded" state (prevents unnecessary API calls).

Is it possible to somehow trigger "cancel" event after I unmount the component? React-router triggers @@router/LOCATION_CHANGE but I don't know how to handle that event (if it is possible) in some nested subcomponent. If I understand redux-elm correctly the subcomponents handles only actions which has proper prefix e.g. if it was OrdersPage.Order.LOCATION_CHAGE.

What's the drawback of handling "phantom" API responses? Let say I'm entering orders list page and then (without waiting for list to load) I go to the single order form then I click "back" button and switch to the orders list. It will be nice to see orders list properly rendered (by handling "phantom" API response) instead of waiting for the new API request to complete.

tomkis commented 8 years ago

@jmarceli there are currently two local actions dispatched which are not exported but should be. For now please use them as String. While handling @@redux-elm/Unmount you could do any cleanup (eg. reset the loading flag)

We will export those two actions in the next version.

jmarceli commented 8 years ago

Thanks, I forgot about those two actions.

More important for me is why do you think that cancelling unmounted components sagas is good idea? (If you are convinced about that reading text below is pointless) From my perspective (handling API calls) it seems to be more efficient if I don't cancel them. If I do I just loose API response data (e.g. entire orders list which is returned even if I cancel the saga). I think that changing default behaviour of the Updater toReducer() method regarding Saga cancelling is too complicated. It would be much easier if toReducer() will have an argument e.g. toReducer(cancelSaga = false) which may prevent Saga cancellation. Another solution is to compose toReducer method by other methods (one of them might be cancelSagas which would be easier to override if someone wants to).

My current workaround/hack is to extend Updater getActionPrefix(action) method but it is too hacky to be treated as a recommended solution:

export default class CustomUpdater extends Updater {
  getActionPrefix(action) {
    // HACK which prevents from cancelling Sagas after Unmounting component
    if (action && action.type === Unmount && action.effectExecutor) {
      return '@@SAGA_NOT_CANCELED@@';
    }

    return super.getActionPrefix(action);
  }
}
tomkis commented 8 years ago

More important for me is why do you think that cancelling unmounted components sagas is good idea?

It's about tradeoffs, I've had many issues where ongoing requests mutated something in the appstate which wasn't desired or even worse, could trigger another asynchronous long running transaction.

toReducer(cancelSaga = false)

I don't think that this is a good idea, because in next version we are trying to decouple concerns however this would mean coupling Updater with Sagas.

My current workaround/hack is to extend Updater getActionPrefix(action) method but it is too hacky to be treated as a recommended solution:

Please, do not do it this way, because Sagas are not only cancelled but most importantly deleted from memory, the implementation is there for purpose. It's very important to cleanup Sagas upon component unmounting.

Your solution may present two problems as soon as your application grows in complexity:

1) Leaking memory 2) Multiple instances of Saga for single Component (eg. when you remount the Component 3 times and then your Saga uses put effect, it will put 3 actions instead of 1)

jmarceli commented 8 years ago

OK. Thanks for clarification. I see I'll have to handle component unmounting action.