Add ability to create daemon sagas

[afitiskin]

This PR fixes #1183 with 100% backward compatibility. Now we are able to create daemon sagas. Daemon sagas run only once (when loaded), next time injector ignore them. So, we don't need to track LOCATION_CHANGE and manually stop simple sagas to prevent duplicates: we just need to set isDaemon attribute of saga to true and that's it!

Before:

/**
 * Watches for LOAD_REPOS actions and calls getRepos when one comes in.
 * By using `takeLatest` only the result of the latest API call is applied.
 */
export function* getReposWatcher() {
  yield fork(takeLatest, LOAD_REPOS, getRepos);
}

/**
 * Root saga manages watcher lifecycle
 */
export function* githubData() {
  // Fork watcher so we can continue execution
  const watcher = yield fork(getReposWatcher);

  // Suspend execution until location changes
  yield take(LOCATION_CHANGE);
  yield cancel(watcher);
}

// Bootstrap sagas
export default [
  githubData,
];

After:

/**
 * Watches for LOAD_REPOS actions and calls getRepos when one comes in.
 * By using `takeLatest` only the result of the latest API call is applied.
 */
export function* getReposWatcher() {
  yield fork(takeLatest, LOAD_REPOS, getRepos);
}
getReposWatcher.isDaemon = true;

// Bootstrap sagas
export default [
  getReposWatcher,
];

[coveralls]

Coverage increased (+0.03%) to 98.605% when pulling 7433d02 on afitiskin:dev into 2e19191 on mxstbr:dev.

[coveralls]

Coverage increased (+0.03%) to 98.605% when pulling 7433d02 on afitiskin:dev into 2e19191 on mxstbr:dev.

[mxstbr]

@justingreenberg what do you think about this change?

[justingreenberg]

@afitiskin awesome work :)

@mxstbr this change will definitely achieve stated goal, but i'm still reluctant to move any additional logic into our internal utilities... inevitbably newcomers will mistake our isDaemon flag as part of redux-saga api (since this is such an elegant solution :)) which will only lead to more confustion. i know i sound like a broken record at this point but saga duplication in this case is not a bug, it just forces people to understand the redux-saga API... IMO people should be customizing their own utilities in exactly this way, but based on their use-case not our demo app

tl;dr - this is awesome work by @afitiskin, but i would probably wait for next major release and move helpers into external package as discusssed

[mxstbr]

Sure, sounds good to me.

Thank you @afitiskin for taking the time to kick off this discussion and taking the time to submit a PR!

[gihrig]

This looks awesome 🎉

Between the switch to Jest, replacing Post-CSS with Styled-components and now Daemon sagas... With a little documentation for Daemon sgas, I'd say we're very ready for 3.4.0.

Thoughts?

[mxstbr]

Did you see that I closed this PR? 😉 But we should def ship a new version asap!

[gihrig]

I did, but I'm suggesting that it be included in the same (next?) release that also contains Jest and Styled Components. Those three make a pretty big change, I think worthy of at least a minor version bump.

[mxstbr]

Yeah need to publish 3.4 already! Will get to that soon.

[gihrig]

@justingreenberg @mxstbr

inevitbably newcomers will mistake our isDaemon flag as part of redux-saga api (since this is such an elegant solution :)) which will only lead to more confustion.

I see the issue of appearing to 'pollute' redux-saga api, but this issue is a frequent source of confusion as it is. I seriously think the confusion caused by this change is the lesser evil.

I vote to reopen and merge this PR. Regardless, documentation is the answer to either problem.

@afitiskin On the redux-saga project there are (currently) 15 issues returned on a search for "duplicate". Consider opening a PR there to make isDaemon an official part of the redux-saga API ?

[justingreenberg]

@gihrig i'm sorry this issue is still coming up so much... it's frustrating because i so want it to be a learning experience, after all it's just javascript 😜 anyway this specific issue has come up so many times, it probably sensible to address it. with respect to this PR, i can say it is the most elegant of the proposed "solutions" i've seen.

what bugs me is that the "problem" is described as a saga duplication issue, when it isn't.

the real issue is that users want to bind daemon sagas to component lifecycle, in the case of our demo it's a route handler loading data. our current implementation (even after merging this PR) was written for the demo—there are better solutions for route-based data loading

https://github.com/jfairbank/redux-saga-router

if you know your route map/data needs in advance, this tool syncs directly with history/router

// sagas.js
import { router } from 'redux-saga-router'

function * rootSaga () {
  yield * router(history, {
    * '/' () {
      yield fork(takeLatest, LOAD_REPOS, getRepos)
    }
  })
}

https://github.com/threepointone/react-redux-saga

this is super simple, component/react lifecycle based

// containers/HomePage/index.js
export class HomePage extends React.PureComponent {
  render() {
    //...
    return (
      <article>
        <Saga saga={getReposWatcher} /> // cancelled on unmount
        // ...

even lifecycle hooks combined with store.runSaga may be a sensible solution. the real motivation behind injectAsyncSagas actually didn't really have anything to do with managing sagas, it was to create a closure for System.import so that saga logic could be bundled with route module

edit: references for discussion -

• automatically cancel running sagas on location change automatically cancel running sagas on location change #1106 (comment)
• Altered injectAsyncSagas to prevent injecting duplicate sagas Altered injectAsyncSagas to prevent injecting duplicate sagas #744 (comment)
• Code Splitting/Async route issue with query string for same route Code Splitting/Async route issue with query string for same route #888 (comment)
• Switch pages making sagas repeat inject Switch pages making sagas repeat inject #826

[gihrig]

@justingreenberg As always, thanks for the deeper insight ✨

Do you think it would be a good idea to integrate a redux-saga-router based solution into the example app as a demonstration of best practice? Pros/cons?

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.