[Question/Documentation] How to deal with "many" sagas #178

Closed
eMerzh opened this Issue Mar 8, 2016 · 23 comments

Projects

None yet

8 participants

@eMerzh
eMerzh commented Mar 8, 2016

Hi,

I'm planning to use saga, but i was wondering,
as a SPA, i got somewhat "many" possible API calls
So when i discovered that you have to mention all your sagas in the store creation,
i feared about performance or managements issues.

What's the recomended way of dealing with 10+/100+ sagas, is it a good idea?
should i try to group them as much a possible ?

@pke
Contributor
pke commented Mar 8, 2016

I have to chim in here and ask why I see so many saga examples that export

export default sagas = [
  yield [
    fork *saga1,
    fork *saga2,
    ...
  ]
]

What's the benefit? Shouldn't a simple array of saga func generators be sufficient to hand into the saga middleware?

@eMerzh
eMerzh commented Mar 9, 2016

Yeah i use the same kind of things
in my sagas.js

export default function* root() {
    yield [
        fork(watchLogin),
        fork(watchLogout)
    ];
}
@pke
Contributor
pke commented Mar 9, 2016

Why not just return the sagas without forking them? I would guess the saga middleware forks each saga in the array anyway

@yelouafi
Member
yelouafi commented Mar 9, 2016

@pke it's mostly for convenience see #160

@eMerzh as @pke said forked and startup Sagas are started with the same mechanism function under the hood so I don't think there is a perf. difference between the 2 methods. As mentioned in #160 the only difference is that using fork you get a reference to a Task object so you can interact with is (cancel it, inspect if it's running etc ...)

@yelouafi yelouafi added the question label Mar 9, 2016
@eMerzh
eMerzh commented Mar 9, 2016

Thanks for the reply :)
my initial question, was more about, is it normal to have "a lot of sagas"
like ... 1 per api call / endpoint or smth ?
so if the application calls 20 distincts endpoints like (create / update / delete / list of object A, ....) i should have this much sagas?

or is it a code smell? then what's the recommended way ?

@yelouafi
Member
yelouafi commented Mar 9, 2016

it depends on the use case; but if the requirements for all APIs are the same (e.g. dispatch success/error actions on each api response, handle logout errors ...) I'd start with one parametrized saga. This way if the requirements are going to change (which is likely to happen at the beginning) I'll only have to change in one place.

If you have for example an api call that's called in many places then you may define a saga that just delegates the generic request saga

function* apiSaga(fn, args, successAction, errorAction) {
  try {
    const { response } = yield call(fn, ...args)
    yield put(successAction(response)
  } catch({ error }) {
    yield put(errorAction(error)
  }
}

// this one is used on many places
function* fetchUser(userId) {
  yield* apiSaga(api.fetchUser, [userId], actions.userFetchSuccess, actions.userFetchError)
}

But beware of premature factoring, do it if you're only confident that all your api calls (or most of them) are handled the same way.

@pke
Contributor
pke commented Mar 9, 2016

@yelouafi Speaking of such factoring, that what I am using atm in my app:

import { takeEvery } from "redux-saga"
import { put, call } from "redux-saga/effects"
import { requestAsync } from "./api"
import { REQUEST } from "./apiActions"

function *performRequest(request) {
  try {
    const { payload, response } = yield call(requestAsync, request.method, request.path, request.data, request.headers)
    const successAs = request.successAs ? request.successAs : "payload"
    yield put({ type: request.successAction, payload: {
      [successAs]: payload,
      response,
      timestamp: Date.now()
    }})
  } catch (error) {
    yield put({
      type: request.errorAction,
      error: true, payload: {
        error,
        request,
        timestamp: Date.now()
      }
    })
  }
}

export function *apiRequest() {
  yield *takeEvery(REQUEST, performRequest)
}

It is used by other modules like this:

function *fetchCities() {
  yield put(apiAction(getCities(), REFRESH_SUCCESS, REFRESH_ERROR, "cities"))
}

function *refreshCities() {
  yield log(fetchCities(), "Initial city refresh")
  yield *takeLatest(REFRESH, fetchCities)
}

export default [
  refreshCities
]

Its slightly different than your example, as the city saga does not import the apiSaga directly but creates an REQUEST action. I am not sure if your model or mine is "better". What are the pros & cons?

@eMerzh
eMerzh commented Mar 10, 2016

Thanks @yelouafi for your reponse.

so if i understand correctly, i still have to add a 3rd function that mostly

function *watchUserSaga() {
  yield* takeEvery("USER_FETCH_REQUESTED", fetchUser);
}

right?

and i have to add the watchUserSaga + fetchUser for each of my endpoints?

@yelouafi
Member

and i have to add the watchUserSaga + fetchUser for each of my endpoints?

Yes but you can also setup multiple watchers on the same place

function *watchMany() {
  yield [
    takeEvery("USER_FETCH_REQUESTED", fetchUser),
    takeEvery("ANOTHER_ACTION", anotherSaga),
    ...
  ]
}
@pke
Contributor
pke commented Mar 11, 2016

@yelouafi and what would be the best way to have them all being canceled on LOGOUT action?

Like this?

function* watchManyWhileLoggedIn(access_token) {
  yield race(
    take("LOGOUT"),
    watchMany(access_token)
  )
}

or in a more generic way

function* runUntil(effect, ...sagas) {
  yield race(effect, ...sagas)
}

function* runUntilAction(actionName, ...sagas) {
  yield runUntil(take(actionName), sagas)
}

runUntil(take("LOGOUT"), watchManyWhileLoggedIn(access_token))
runUntilAction("LOGOUT", watchManyWhileLoggedIn(access_token))
@yelouafi
Member

@pke You'll have to use the low level API for that

function *takeEvery(watchAction, cancelAction, worker) {
  const taskArray = []
  while(true) {
    const action = yield take([watchAction, cancelAction])
    if(action.type === cancelAction)
      yield tasks.map(t => cancel(t))

    else {
      const task = yield fork(worker, action)
      taskArray.push(task)
      task.done(() => remove(taskArray, task))
    }
  }
}

function *watchMany() {
  yield [
    takeEvery("USER_FETCH_REQUESTED", "LOGOUT", fetchUser),
    takeEvery("ANOTHER_ACTION", "LOGOUT", anotherSaga),
    ...
  ]
}
@pke
Contributor
pke commented Mar 11, 2016

hmmm forking all the worker actions and race them against the cancelAction would not work?

@yelouafi
Member

Sure It would and the code will also be more declarative, you won't even need to create a custom helper. OTOH you'll have to create a task-watcher for each forked worker (not a big deal on most cases)

function* cancellableWorker(worker, cancelAction, workerAction) {
  yield race([
    call(worker, workerAction),
    take(cancelAction)
  ])
}

function *watchMany() {
  yield [
    takeEvery("ACTION", cancellableWorker, fetchUser, "LOGOUT")
    ...
  ]
}
@yelouafi yelouafi closed this Mar 16, 2016
@slorber
Contributor
slorber commented Jun 30, 2016

@yelouafi by chance do you know why this does not work?

yield stamples.map(stample => {
    call(watcherGenerator,stample);
});

while this works:

yield call(watcherGenerator,stample);

I don't understand :D

@yelouafi
Member
yield stamples.map(stample => {
    /* return */ call(watcherGenerator,stample);
});
@slorber
Contributor
slorber commented Jun 30, 2016 edited

:O obviously :)

by chance is there a way to add a warning for such cases when we are yielding an array those values are not effects/promises? it would probably be helpful to others

thanks

@yelouafi
Member
yelouafi commented Jun 30, 2016 edited

Maybe adding a warning when an undefined value is yielded. more general and can also cover the above case

@slorber
Contributor
slorber commented Jun 30, 2016

hmmm actually one might want to map and not return effect for all the elements of the array (I almost did try that).

I certainly can use _.compact to remove undefined values from the array anyway

@TeodorKolev

I want to use redux-saga for API calls. Since I have many different url calls how can I achieve that? Basicaly I use this example, but I dnon't know how to multiply it. Is there any tutorials for that? Just to be sure: I have two different buttons in different components that make API calls.

@davidwparker

@TeodorKolev - I've modified your example to show using two different API calls. You should be able to expand upon that to make it work: http://www.webpackbin.com/VkdjuU02Z

@TeodorKolev

@davidwparker Great thanks man. Appreciated! If you want add that links as answer of this. I will accept it, because it really helps me.

@jamiesunderland

I've experienced the same problem. My solution was to create a sagas.js file next to app.js that acts as the root saga and then maps over the imported sagas. so for example

sagas.js

import { fork } from 'redux-saga/effects';
import MySaga from './path/to/MySaga';
import MyOtherSaga from './path/to/MyOtherSaga';

const sagas = [
  ...MySaga,
 ...MyOtherSaga
];

export default function* root() {
  yield sagas.map(saga => fork(saga)); 
}

path/to/MySaga.js

export default [myWatchSagaForSomething, myWatchSagaForSomethingElse];

app.js

import Sagas from './sagas';
/*
lot of other stuff ...
*/
sagaMiddleware.run(Sagas);
@granmoe
Contributor
granmoe commented Jan 18, 2017

We've ended up doing the same thing as @jamiesunderland. We do it within a modified ducks pattern, where the saga code is all contained within our ducks. Each duck exports a sagas array, then we yield sagas.map(startSaga) in our rootSaga (startSaga spawns each saga and has some top level custom logic).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment