New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saga Practice #158

Closed
bradennapier opened this Issue Jun 11, 2016 · 28 comments

Comments

7 participants
@bradennapier

bradennapier commented Jun 11, 2016

First of all - absolutely awesome love what you guys have done here.

I have noticed your Saga's use yield call rather than yield fork. It is my understanding this is not the right practice unless you are using the newer "takeEvery" because any actions that occur before the call has completely finished will not be received - in this case fork should instead be used.

I could be incorrect but this is how I had to set things up in our React App otherwise many issues would arise. Perhaps either the new takeEvery commands should be used or the generator / example should show fork to be inline with the Saga best practices?

Granted I understand something like "logiN' isn't gonna receive multiple calls like this, but for a boilerplate that is meant to teach best practices, I can see how this would end up causing someone that is newer to Sagas have issues that are fairly hard to debug / understand.


// a daemonized version which waits for LOGIN_ATTEMPT signals
export function * watchLoginAttempt () {
  // daemonize
  while (true) {
    // wait for LOGIN_ATTEMPT actions to arrive
    const { username, password } = yield take(Types.LOGIN_ATTEMPT)
    // call attemptLogin to perform the actual work
    yield call(attemptLogin, username, password)
  }
}

Changes To:

// a daemonized version which waits for LOGIN_ATTEMPT signals
export function * watchLoginAttempt () {
  // daemonize
  while (true) {
    // wait for LOGIN_ATTEMPT actions to arrive
    const { username, password } = yield take(Types.LOGIN_ATTEMPT)
    // call attemptLogin to perform the actual work
    yield fork(attemptLogin, username, password)
  }
}
@GantMan

This comment has been minimized.

Member

GantMan commented Jun 11, 2016

Awesome feedback and issue! yes, we probably need to revisit this in the ignite project.

Our own @skellock will have to sign off, he's our saga guru, but I agree with what you're saying.

@skellock skellock self-assigned this Jun 11, 2016

@skellock

This comment has been minimized.

Contributor

skellock commented Jun 11, 2016

I think you'd want to change it to fork if you want to "attempt to login more than once". I don't think this is something you'd want to do. You could prevent that from happening by disabling the UI as well while attempting is underway.

But your bigger point of "teaching best practices" is reaching me.

We sort of laid down the saga strategy a few months ago. Before takeEvery and takeLatest hit the scene that wraps this pattern.

We need a refresh with this and a few other saga-related things:

  1. Start using takeEvery & takeLatest
  2. Remove the worker/watcher pattern and only leave a worker which becomes the default export.
  3. Make "workers" accept an action (and optional api) if it needs it. (as opposed to destructuring up at the watcher level).
  4. Recommend 1 saga function per file.
  5. Simplify testing and document (perhaps a blog post) on the traps of saga testing.

Thanks for calling this out @bradennapier . We'll make this better.

@bradennapier

This comment has been minimized.

bradennapier commented Jun 14, 2016

Perhaps you guys might like to consider something like the SagaProcess classes which allow you to run local and global reducers to daemons / processes. It has been great on our end for cleaning things up.

import { take, put, call, fork } from 'redux-saga/effects'
import Immutable from 'seamless-immutable'

import Process from '../Process'

import { TYPES } from './config'

const {
  TCP_OPEN,
  TCP_CONNECTED
} = TYPES

import {
  PROCESS_RENDERS
} from '../../Types/Process'

class TCP extends Process {

  constructor(props) {
    super(props)
    this.state = {}
  }

  static config = {
    path: '/',
    reduce: 'tcp',
  };

  static initialState = Immutable({
    _openSockets: 0
  });

  static cancelTypes = [ ]

  static actionRoutes = {
    [TCP_OPEN]: 'connect'
  };

  static reduce = {
    // A TCP Socket has Opened...
    [TCP_OPEN]: (state, action) => state.merge({ [action.id]: { status: 'connecting', ip: action.ip, port: action.port } }),

    // A TCP Socket has Connected...
    [TCP_CONNECTED]: (state, action) => state.merge({ [action.id]: { ...[action.id], status: 'connected' } }),
  };


  * connect(action) {
    const { ip, port, id } = action
    // TCP Connection Here
  }

  * connected(socket) {
    const { id } = socket
    yield put({ type: TCP_CONNECTED, id })
  }

  * receive(socket, data) {
    const { id } = socket
    yield put({ type: TCP_RECEIVE, id, data })
  }

  * startObserver(action) {
    /*
      The observer can be called using this.createObserver and allows you to turn
      any node style callbacks into saga daemons:

       Firebase Callback Example:

         const { getNext, onData, onCancel } = this.createObservable(`Observer-Name`)
         const observerID = ref.on(type, onData)

         try {
            while (true) {
              const data = yield call(getNext)
              yield* this.processReceived(data, type)
            }
          } catch(error) {
            console.warn('Catch Called in Observer', error.message)
          } finally {
            if (yield onCancel()) {
              ref.off(type, observerID)
            }
          }
    */
  }

  * processStarts() {
    // This is run on startup, any daemon-like functionality can be placed here
    // if required.  If this Process is only for monitoring of Redux Actions
    // you can leave this empty and add the actionRoutes & reduction that should occur
  }

  * processCancels() {
    // This is run when the tasks associated with this Process are being cancelled.
  }
}

export default TCP

@skellock

This comment has been minimized.

Contributor

skellock commented Jun 21, 2016

That's really interesting how you've done this. Thanks for sharing.

Also, I had no idea that you could define generator functions like this.

Any special babel plugin required? or are you just using the react-native-preset?

Have you looked into the new Channel support in redux-saga?

@bradennapier

This comment has been minimized.

bradennapier commented Jun 21, 2016

Yeah it has worked out really nicely for us thus far. It is really quite powerful and it automatically builds the reducers for us and allows us to map actions and everything in a completely automated way while following best practices.

Yeah I have played with the channels, for the most part they aren't needed if you know how to implement everything properly but probably makes sense to implement them in some ways. We built our own implementation of Action Channels which includes buffering and some extra features which is called by making a this.createObserver(name) call within the class. It is a cancellable promise and works with anything that has a callback parameter which will be called any number of times in the future.

This also has a task system built in for saving tasks to ID's and cancelling them automatically as-needed.

Here is an example of a working "Device" SagaProcess that monitors orientation as well as other device specifications. SagaProcesses are set to start after the stores have been rehydrated so we are also able to use that to compare specs and deploy only when changed, etc (so we know to deploy to our servers):

import { take, put, call, fork, select, apply } from 'redux-saga/effects'
import Immutable from 'seamless-immutable'
import Process from '../Process'
import { Dimensions } from 'react-native'
import Orientation from 'react-native-orientation'
import DeviceInfo from 'react-native-device-info'

import { TYPES } from './config'
const {
  DEVICE_ORIENTATION_CHANGES,
  DEVICE_NEW_SPECS,
} = TYPES

class AccountDeviceProcess extends Process {

  constructor(props, State) {
    super(props, State)
  }

  static config = {
    path: '/',
    reduce: 'device'
  };

  static initialState = Immutable({
    orientation: 'UNKNOWN',
    height: undefined,
    width: undefined,
    specs: {},
  });

  static cancelTypes = [];

  static actionRoutes = {
    [DEVICE_NEW_SPECS]: 'handleUpdateDeviceSpecs'
  };

  static reduce = {

    [DEVICE_ORIENTATION_CHANGES]: (state, action) => state.merge({
      orientation: action.orientation || 'UNKNOWN',
      width: action.width,
      height: action.height,
    }),

    [DEVICE_NEW_SPECS]: (state, action) => state.merge({
      specs: action.deviceSpecs
    }),

  };

  * handleOrientationChange(orientation) {
    const { width, height } = Dimensions.get('window')
    yield put({type: DEVICE_ORIENTATION_CHANGES, orientation, width, height})
  }

  * getDeviceInfo() {
    /*
      We iterate through the Device Specifications to determine if we should
      update the server or not based upon previous values associated with this
      account.
    */
    const deviceSpecs = {
      uid: DeviceInfo.getUniqueID(),
      mfg: DeviceInfo.getManufacturer(),
      model: DeviceInfo.getModel(),
      versionID: DeviceInfo.getDeviceId(),
      osVersion: DeviceInfo.getSystemVersion(),
      appVersion: DeviceInfo.getReadableVersion(),
      deviceName: DeviceInfo.getDeviceName(),
      deviceLocale: DeviceInfo.getDeviceLocale(),
      deviceCountry: DeviceInfo.getDeviceCountry()
    }

    const previousDeviceSpecs = yield select(this.state.deviceSpecs)
    let shouldUpdateSpecs

    if (previousDeviceSpecs) {
      for (let spec of Object.keys(deviceSpecs)) {
        const prevSpec = previousDeviceSpecs[spec]
        if (!prevSpec) {
          shouldUpdateSpecs = true
          break
        }
        const nextSpec = deviceSpecs[spec]
        if (prevSpec !== nextSpec) {
          shouldUpdateSpecs = true
          break
        }
      }
    } else {
      shouldUpdateSpecs = true
    }

    if (shouldUpdateSpecs) {
      console.log('Update Device Specs!')
      yield put({type: DEVICE_NEW_SPECS, deviceSpecs})
    }
    console.log(deviceSpecs)

  }

  * handleUpdateDeviceSpecs(action) {
    console.log('Handle Update Device Specs!')
    console.log(action)

  }

  * orientationObserver() {
    const { getNext, onData, onCancel } = this.createObservable(`DeviceOrientationObserver`)
    const observerID = Orientation.addOrientationListener(onData)
    try {
      while (true) {
        const { values } = yield call(getNext)
        for (let value of values) {
          yield fork(::this.handleOrientationChange, value)
        }
      }
    } catch(error) {
      console.warn('Catch Called in Observer', error.message)
    } finally {
      if (yield onCancel()) {
        console.log('Orientation Listener Cancelled')
        Orientation.removeOrientationListener(onData)
      }
    }
  }

  * processStarts() {
    // This is run on startup, any daemon-like functionality can be placed here
    // if required.  If this Process is only for monitoring of Redux Actions
    // you can leave this empty and add the actionRoutes & reduction that should occur
    const initialOrientation = Orientation.getInitialOrientation()
    yield apply(this, this.handleOrientationChange, [ initialOrientation ])
    const orientationTask = yield fork(::this.orientationObserver)
    yield apply(this, this.saveTask, [orientationTask, 'device', 'orientation'])
    yield apply(this, this.getDeviceInfo)
  }

  * processCancels() {
    // This is run when the tasks associated with this Process are being cancelled.
  }
}

export default AccountDeviceProcess

Nope just the preset I believe

@bradennapier

This comment has been minimized.

bradennapier commented Jun 21, 2016

Then in reducers we iterate:

// We iterate through each process category then further
// iterate through each Process.  If they declare a reduction
// facility we will instanstiate the reducer here.
for (const _category of Object.keys(Processes)) {
  const category = Processes[_category]
  for (const _process of Object.keys(category)) {
    const process = category[_process]
    const { config, reduce, initialState, actionRoutes } = process
    if (config && config.reduce && initialState && reduce) {
      reducers[config.reduce] = createReducer(initialState, reduce)
    }
  }
}

// glue all the reducers together into 1 root reducer
export default combineReducers({
  ...reducers,
  account: AccountReducer,
  navigationState: NavigationReducer,
  temp: TempReducer
})

and we do similar within our Saga Processor which reads all the classes in all the folders and starts them up:

export default function * root() {
  for (const _category of Object.keys(Processes)) {
    const category = Processes[_category]
    for (const _process of Object.keys(category)) {
      const processClass = category[_process]
      const { config } = processClass
      const SagaProcess = new processClass(config, State)
      yield fork(::SagaProcess.processInit, processClass)
    }
  }
}

State is a variable which holds our selectors so we can do

yield select(this.state.pushToken)

and it is really just calling the selector:

export default {
  isPaired: state => state.account.isPaired,
  isRehydrated: state => state.temp.rehydrated,
  pushToken: state => state.notifications.pushToken,
  deviceSpecs: state => state.device.specs,
  appLastOpened: state => state.app.lastOpened,
  appOpenHistory: state => state.app.openHistory
}

anyway, just thought I would share! Processes are fairly easy concepts to reason about so it works well... we still use pure sagas for simple things that cant be attributed to a class like this, but the little utilities are nice to have - especially the actionRoutes

@ahmed1490

This comment has been minimized.

ahmed1490 commented Jun 23, 2016

quote

Make "workers" accept an action (and optional api) if it needs it. (as opposed to destructuring up at the watcher level).

@skellock Can you explain a bit clearly with a rough code? Thanks! :)

@skellock

This comment has been minimized.

Contributor

skellock commented Jun 27, 2016

Sure.

Use the same interface as takeLatest and takeEvery.

// a worker (e.g. loginRequestSaga.js)
export default function * (action) {
  // do the saga thing in here
}

And from our Root Reducer, queue up "subscriptions" there.

// "watcher" behaviour
yield [ takeLatest(Actions.LOGIN_REQUEST, loginRequestSaga) ]

Root becomes an "switchboard" and sagas become easy to hookup here, or be called directly from other sagas.

I'm currently iterating on this idea in my current project. It'll be refined before I'm ready to PR.

Still a few issues to solve. The big one is: how to pass shared resources to worker (like the API). Do we use a factory function like we do now? Do we add it to the action object via middleware? I really don't know just yet.

@ahmed1490

This comment has been minimized.

ahmed1490 commented Jun 28, 2016

I like the idea of root being a "switchboard".

One option to pass resources would be pass it through the effect itself.

yield [ takeLatest(Actions.LOGIN_REQUEST, loginRequestSaga, {api}) ]
export default function * loginRequestSaga (resources, action) {
  // use resources.api
  // do the saga thing in here
}

umm.. not so clean yet though.

@bradennapier

This comment has been minimized.

bradennapier commented Jun 28, 2016

Hmm, isn't that exactly what I have done with the "Saga Process" I posted above? I may be misunderstanding your intent but as far as I can tell it's what you are describing with the "actionRoutes"

@skellock

This comment has been minimized.

Contributor

skellock commented Jun 28, 2016

@bradennapier It might be... do you have a repo with some working code I could go through?

@skellock

This comment has been minimized.

Contributor

skellock commented Aug 2, 2016

Ok, a month later and I've had a chance to look at this.

I've got something in place now that looks like this:

Root Saga

export default function * root () {
  yield watch(Types.SOME_ACTION_TYPE, someSaga)
}

Saga

The saga in this case is just a single saga exported as default. No worker. No watcher. The root saga is responsible for hooking up which message triggers things.

Just like how the root reducer names her reducer's locations.

export default function * someSaga (action) {
  yield call(go)
}

1 Saga per file.

Passing API to Sagas

If we need to pass in things like API into the saga, we just add it to the parameter list:

export default function * someSaga (action, api) {
  yield call(api.login, 'steve', 'password')
}

Then the root saga would pass it when it should watch:

yield watch(Types.SOME_ACTION_TYPE, someSaga, api)

That's it.

Watch.js source code

function * watch (type, saga, ...rest) {
  function * sagaWatcher () {
    while (true) {
      const action = yield take(type)
      yield call(saga, action, ...rest)
    }
  }
  yield fork(sagaWatcher)
}

I'd like to add this reduxsauce.

This cleans up a ton of boilerplate code and makes calling sagas great again.

@GantMan

This comment has been minimized.

Member

GantMan commented Aug 2, 2016

image
Don't let the picture fool, you it's photoshop, that hat doesn't actually exist!

@mmazzarolo

This comment has been minimized.

Contributor

mmazzarolo commented Aug 2, 2016

@skellock nice approach and great discussion.
Just a note: I used a similiar approach in the last month and it worked great for me until I started having a lot of sagas (for authentication and authorization for example) and 1 saga per file became overkill.
I'm currently grouping sagas by domain/reducer and I'm pretty happy with it for now.
I also started using sagas for handling stuff like intercepting errors and showing alerts/snackbar:

// index.js
 takeEvery(authActionTypes.LOGIN_FAILURE, uiSagas,showErrorAlert),
 takeEvery(authActionTypes.SIGNUP_FAILURE, uiSagas.showErrorAlert)

// uiSagas.js
import { call } from 'redux-saga/effects'
import { Alert } from 'react-native'
import i18n from '../i18n/'

export function* showErrorAlert (action) {
  const { error } = action
  yield call(Alert.alert, i18n.t('ERROR_TITLE'), error)
}

Which may sound "dirty" but it's working well on keeping all the logic and effects in the sagas (hope I won't have to go back on my word 🎅 )... and if more abstraction is needed you can always create a service/api for handling that stuff (for example services/handleError.js).

@mmazzarolo

This comment has been minimized.

Contributor

mmazzarolo commented Aug 2, 2016

On a side note: you already considered trying the ducks style for reducers/actions, right?
Just asking because I always disliked it but lately it is "almost" working for me since all my actions are dumb (just like this) and the interesting stuff happens in sagas.
Btw guys you made a SOLID boilerplate, I'm still in love with its structure so far 💇... and discussion like this one are top notch (and unusual in boilerplate repos). Good job.

@skellock

This comment has been minimized.

Contributor

skellock commented Aug 2, 2016

Thanks for the feedback @mmazzarolo!

re: ducks...

we kicked the around the idea of splitting up the app like that. Wasn't well received.

Personally, I think I might be clinging too tightly to "all my actions in one file", "all my types in one file", a single root reducer, a single root saga. Dividing by "tech" seems to be where my head goes first.

I do see the merits in it, just probably not all the advantages. Would love to see more examples out there.

Re: 1 Saga Per File

I think this is an awesome setup. You're right. Lots of sagas. But they all have the same interface, and they're all capable of calling each other directly without going back to thru redux. Composable.

I did a project recently where, upon login, you head in 1 of 2 different flows. Some steps of the flows were shared. I communicated with the steps via put and take, and it was nasty.

Re: alerts

I like that idea a lot. We're using this technique now on my current project. We've got a scenario where any response code back from the API that's 500-series or network or timeout will show an alert.... else the alert is the responsibility of the calling saga (for 400-series).

Here's part of that code in action:

import R from 'ramda'
import { TIMEOUT_ERROR, UNKNOWN_ERROR, SERVER_ERROR, CONNECTION_ERROR, NETWORK_ERROR } from 'apisauce'
import { Alert } from 'react-native'

// TODO: don't forget to change these when we talk to the client (or Justin)
const DEFAULT_TIMEOUT_MESSAGE = 'Sorry, the server took too long to respond.  Please try again.'
const DEFAULT_SERVER_MESSAGE = 'Sorry, bad things happened on the server.'
const DEFAULT_CONNECTION_MESSAGE = 'Sorry, the server is not available.  Please try again.'
const DEFAULT_NETWORK_MESSAGE = 'Sorry, unable to reach the server, double check your Internet connection.'
const DEFAULT_UNKNOWN_MESSAGE = 'Sorry.  Something strange just happened.'

// attach our monitor for a specific problem, and show a message
const registerMonitor = (api, problem, message) =>
  api.addMonitor(response => {
    if (response.problem === problem) {
      Alert.alert(message)
    }
  })

// timeouts
const monitorTimeout = (api, message = DEFAULT_TIMEOUT_MESSAGE) => registerMonitor(api, TIMEOUT_ERROR, message)

// 5xx HTTP errors
const monitorServer = (api, message = DEFAULT_SERVER_MESSAGE) => registerMonitor(api, SERVER_ERROR, message)

// flakey server
const monitorConnection = (api, message = DEFAULT_CONNECTION_MESSAGE) => registerMonitor(api, CONNECTION_ERROR, message)

// flakey internet
const monitorNetwork = (api, message = DEFAULT_NETWORK_MESSAGE) => registerMonitor(api, NETWORK_ERROR, message)

// any other strange error
const monitorUnknown = (api, message = DEFAULT_UNKNOWN_MESSAGE) => registerMonitor(api, UNKNOWN_ERROR, message)

/*
 * Installs some monitors into an Apisauce API which
 * show human-friendly messages when errors occur.
 */
const addMonitors = (api) => {
  if (R.isNil(api)) return

  // attach the monitors to each
  monitorTimeout(api)
  monitorServer(api)
  monitorConnection(api)
  monitorNetwork(api)
  monitorUnknown(api)
}

export default addMonitors

We just plug into our API api (lol) and watch for bad things. That allows the calling sagas to not have to worry about display anomalies... only paths they care about.

@kevinvangelder and I had a discussion about using Sagas to trigger UI-related things. We both agreed it felt a bit dirty at first, but it's really working out well so far. especially with react-native-router-flux's decoupled action-calling behaviour.

Something I'm taking away from what you said here is: I'll probably wrap watch with a try/catch inside. Dealing with uncaught exceptions in sagas (and the stacktrace that it produces) has been a thorn in my side... It wasn't until you wrote your comment did it become clear how I can solve this. Thank you!

Thanks for taking the time to comment. Really appreciate your insights.

@bradennapier

This comment has been minimized.

bradennapier commented Aug 2, 2016

Oh my bad! Didn't see you had replied until all these replies came in! I'll get a boilerplate of the concept for Processes up asap.

I have extended it and i have never been happier with development. I've had a flawless experience with next to no edge cases yet.

'

@mmazzarolo

This comment has been minimized.

Contributor

mmazzarolo commented Aug 3, 2016

I do see the merits in it, just probably not all the advantages. Would love to see more examples out there.

Honestly I'm still doubtful about it too, haha, but it seems to speed up a lot the development for me.
My biggest concern is that some ducks might grow up and turn into giant messes but I have not had this problem since the adoption of sagas.
I also tend to "tweak" a bit the ducks.
Hoping this setup will continue to suit my need and not implode in the future.

But they all have the same interface, and they're all capable of calling each other directly without going back to thru redux. Composable.

I'm probably missing something, but how is that more composable than something like this (sorry the dumb example)?

Re: alerts ...

Wow, sagas are extremely versatile.. I think I'll steal some of that code for handling error responses, thanks!

@skellock

This comment has been minimized.

Contributor

skellock commented Aug 3, 2016

@bradennapier : Thanks man. I didn't quite follow everything you had posted before. Seeing a little more context would really help!

@mmazzarolo:

Thx for sharing that setup.

Honestly I'm still doubtful about it too

You and I share the same mindset: that at any moment, your app design decisions could collapse. Haha. I see you, too, have battle scars.

speed up a lot the development for me

How? Isn't this just about pivoting file locations from "tech" to "domain"? Or does that shift come with the side effect of lifting the mental weight of the project because you're building sub-apps?

how is that more composable than...

It's the same thing. You have the advantage of being in the same file though, whereas, 1 saga per file will have to import dependencies.

concern is that some ducks might grow up...

Where do you put shared components? /App/Shared/Components/Button.js?

Do your domain slices ever cross pollenate? I can see that for sagas. Concern? or no issue?

your root saga

I actually prefer yours to what I'm proposing.

My watch only exists because we have an API object that's initialized & configured. We reuse that object across requests. We could switch that to become self-contained and initialized within api.js, but then it becomes its own island and will need to persist itself (cookies and other headers).

Perhaps that's really not a big deal after all.

I wonder if I could use .bind in the root saga with takeLatest to partially apply it to the saga? Probably not eh? Might not be compatible with generators.

your state tree

I noticed you don't use an immutability library. I personally like that. We talked about that here as well, but decided perhaps playing it safe with one outweighed the honour system approach.

Thanks man, you've given me much to think about.

@mmazzarolo

This comment has been minimized.

Contributor

mmazzarolo commented Aug 3, 2016

You and I share the same mindset: that at any moment, your app design decisions could collapse. Haha. I see you, too, have battle scars.

Hahaha, you're right. I can't count the times I refactored my React and React-Native apps. I must admit that in the end I like refactoring because I always learn new patterns and it's an excuse for trying new stuff.

How? Isn't this just about pivoting file locations from "tech" to "domain"? Or does that shift come with the side effect of lifting the mental weight of the project because you're building sub-apps?

I guess I didn't explain myself well. I don't group everything by domain, I just use ducks/module for handling in a single file action types, action creators, reducers and selectors.
My sagas are all under sagas/ and my screens/containers are not tied 1:1 to ducks, just like yours.
I tried grouping everything by domain (components/containers etc...) in the past but handling domain crossing was a real pain for me.
Do you have any experience worth sharing about grouping everything by domain?

I wonder if I could use .bind in the root saga with takeLatest to partially apply it to the saga? Probably not eh? Might not be compatible with generators.

I can't help here, I started using generators with redux-saga and I haven't experimented much.

I noticed you don't use an immutability library. I personally like that. We talked about that here as well, but decided perhaps playing it safe with one outweighed the honour system approach.

I ditched seamless-immutable because It was giving me a lot of issues (redux-form integration, array handling...).
I should probably start using ImmutableJS again but I'm doing fine without it for now and It would probably slow me down a bit in development.

Thanks man, you've given me much to think about.

Thank you too, I love discussing about this stuff.

@GantMan

This comment has been minimized.

Member

GantMan commented Aug 3, 2016

Woah, you guys are figuring something amazing out. Let me know when you have things locked down and how I can help. Also, I expect a personal lesson... my sagas are good, but not like my photoshop good ;)

@skellock

This comment has been minimized.

Contributor

skellock commented Aug 3, 2016

So much awesome came out of this discussion.

Here's the plan of attack. I passed all of this by with @GantMan and @kevinvangelder. Much to my amazement, we all agree!

I'm going to submit a PR where we pick up this conversation with implementation details. Then you guys can rip me a new one if you don't like what you see.

I award @mmazzarolo 1 gold star.

See you in the PR. Closing.

@skellock skellock closed this Aug 3, 2016

@GantMan GantMan referenced this issue Aug 16, 2016

Closed

State is not persisted on app restart #290

4 of 4 tasks complete
@jgkim

This comment has been minimized.

jgkim commented Aug 16, 2016

@skellock I guess, in the above proposal of the root saga, the yield * expression should be used to delegate to watch generators. Am I missing something?

export default function * root () {
  yield * watch(Types.SOME_ACTION_TYPE, someSaga)
}
@ruskid

This comment has been minimized.

ruskid commented Aug 31, 2016

With this watcher it seems like you don't use takeEvery & takeLatest
Correct me if i am wrong, but maybe it make sense in implementing watchEvery, watchLatest?

@skellock

This comment has been minimized.

Contributor

skellock commented Aug 31, 2016

We're switching to:

return [
  takeLatest(...),
  takeEvery(...)
]
@ruskid

This comment has been minimized.

ruskid commented Sep 1, 2016

Can you provide the final example on how you use sagas?

@skellock

This comment has been minimized.

Contributor

skellock commented Sep 1, 2016

I'm behind on this task. Coming soon. Basically, root saga holds the subscription via takeLatest/takeEvery, and the sagas will just be a collection of generator functions.

@GantMan

This comment has been minimized.

Member

GantMan commented Sep 1, 2016

@ruskid - Steve's been kicking butt on quite a few fronts. He's a :shipit: kinda guy, so we'll see this eventually.

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