Skip to content
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

Build 3rd party plugins that optimize Sagas usage (following this blueprint) [and another for Redux-Observable] #76

Open
bfillmer opened this issue Aug 9, 2017 · 22 comments

Comments

@bfillmer
Copy link

bfillmer commented Aug 9, 2017

Would like to get some ideas from the community on ways to improve the experience of using redux-first-router with redux-saga.

Some thoughts:

  1. Adding a reference to a saga to fire before firing our action dispatch?
  2. Another export from connectRoutes that gets added to our sagas, effectively mapping them to each route?
@djskinner
Copy link

djskinner commented Aug 9, 2017

I've only today discovered redux-first-router and am excited to implement it. For me the switch looks like it will be relatively simple (I never migrated to react-router 4 since it felt like it was going in the wrong direction to me) aside from the saga piece which I haven't sufficiently got my head around yet to be ready to jump into coding it.

I'm using react-router (3) hooks to fetch data on URLs and sagas to perform the async work:

onEnter(nextState, replaceState, cb) => {
 runSaga(loadEntities).done.then(cb)
}

I just imagined being able to transplant that straight into a thunk. Something like:

{
 ENTITIES: {
  path: '/entities/',
  thunk: runSaga(loadEntities).done
 }
}

Ideally I'd want to change this so that I don't have to use the saga but just write an async/await thunk that dispatches the result action. However I have a feeling that loadEntities is used elsewhere by other sagas so need to think how to keep it DRY.

There's also this if you haven't seen it already: #31

@faceyspacey
Copy link
Owner

@bfillmer again, great article! Gave me some ideas. I haven't used Sagas in a while, but I loved them when I did, so I'm not sure of the correct terminology. But regarding:

Adding a reference to a saga to fire before firing our action dispatch?
Is this the idea of having route.saga similar to route.thunk mentioned in my other comments to you?

Another export from connectRoutes that gets added to our sagas, effectively mapping them to each route?

So, I assume this is the part about running that route.saga reference. It would be super slick to do it all within RFR, but obviously it's not a good idea for RFR to also ship saga support. So the question is what exactly should RFR export to make this as absolutely easy as possible for you guys?? A generator function that can be run by you guys in your top level saga?

If you could provide a code example of how you see this working, that would be great?

@iansinnott
Copy link
Contributor

Just wanted to chime in on here because this issue has implications for the larger direction of RFR. Since many people are using many different async solutions if RFR took a stance on async by supporting some solutions out of the box it could risk fragmenting the community around this project. Redux itself is a great example of taking the opposite approach and letting users solve their async problems on their own by being extensible.

Admittedly route.thunk may seem to contradict my point here, but it seems to be the de facto standard for async in redux if you don't have an existing solution and @faceyspacey's points about the 80% use case are fair.

So I would suggest that RFR not provide explicit support for anything outside of thunks, but RFR should work fine with any redux middleware. So if there is a bug that keeps RFR from working along side redux-saga then we should address that, but if it's a matter of developer ergonomics there are other options that would not involve modifying the core of RFR. For example, a redux-first-router-saga package could be created that does any plumbing necessary to provide a more seamless API. Or perhaps RFR itself could become extensible and the thunk functionality could also be separated into its own package.

@bfillmer
Copy link
Author

@iansinnott I don't disagree. My only issue thus far, or rather something that was surprising on first pass, was that my sagas didn't take the initial navigation event on load of the application, but the workaround to check the state for an initialRoute was fairly trivial.

Link to the article mentioned above for reference: https://medium.com/@bryanfillmer/a-redux-first-router-saga-67c2cda9252e

I think what @faceyspacey is probably interested in, and what I admit would be nice, would be to kill the hash I have in src/state/sagas/routes.js and simply have the map of sagas to type happen in src/state/routes.js where the initial creation of the middleware, enhancer, and reducer takes place.

@faceyspacey
Copy link
Owner

@bfillmer yea pretty much--kill the hash. I very much have enjoyed sagas when I've worked with them. It's obvious they resonated with a large # of Redux developers. You hardly ever hear about, say, observables as much as you do Sagas.

That said, Saga apps seem to have too many actions going on. It can get messy.

@bfillmer you know about the initialDispatch function and option right? It was written about for the first time today in reference to SSR:

https://medium.com/faceyspacey/server-render-like-a-pro-w-redux-first-router-in-10-steps-b27dd93859de

But really was first created for the sagas use-case. Let me know if it solves this:

was that my sagas didn't take the initial navigation event on load of the application

@bfillmer
Copy link
Author

@faceyspacey You're right, that did simplify the saga a bit. Instead of the relatively less stable check against the store initially the routing saga was simplified to check only for the route dispatch. I'll update the article.

I'm actually pretty content with the way the boilerplate for this works out. Do you worry that if you add saga-specific hooks, along with the already existing thunk hooks, the library will start to extend beyond a single responsibility and gain a bit of bloat?

@faceyspacey
Copy link
Owner

faceyspacey commented Aug 15, 2017

I don't. I worry about marketing. There's a million great things that have been built with 1000 stars (most of which by the way are now outdated and no longer used), but very few become React Router. I'm seeking for this to become React Router for the Redux crowd so I can continue to support it and other creations. I think it's worthy of it. I think that's how open source actually works--things need to become popular enough to pay the bills.

So if appealing to the Sagas crowd is part of it, it will be done.

It's not the biggest deal, and there can be exceptions to ideology surrounding clean coding and interfaces etc. It's not gonna break anything if it has a sagas option. It's only gonna affect the Sagas people. I just gotta keep it running once it's up.

That said I'm always concerned with bloat, but when it essentially means a "partnership," exceptions can be made. There's very few candidates up for the "partnership" option. Off the top of my head, I can only think of 1 or 2 others besides Sagas.

@faceyspacey
Copy link
Owner

faceyspacey commented Aug 15, 2017

As far as implementation, that's where the bloat potential is. Obviously the best implementation involves kicking off a saga within RFR itself. That we cannot do, as we can't include sagas within RFR, or even/especially the regenerator runtime, as that goes far beyond the "bloat" threshold. I assume that's what you were thinking about.

What's the next best option? Is there a useful function we can return for you to use in your first saga and what would it look like?

Ideally it's more than just a findRoute() function. But that's useful in general. It would look like this:

findRoute(state | locationState, action.type).

@bfillmer
Copy link
Author

@faceyspacey Yeah I was thinking somewhat along the lines of (rough implementation code ahead):

import {connectRoutes} from 'redux-first-router'
import createHistory from 'history/createBrowserHistory'

import {ROUTE_HOME, ROUTE_ABOUT} from 'types'
import {homeSaga} from 'state/sagas/home'

const routesMap = {
  [ROUTE_HOME]: {
    url: '/',
    saga: homeSaga
  },
  [ROUTE_ABOUT]: '/about'
}

const history = createHistory()

export const {
  reducer,
  middleware,
  enhancer,
  sagas,
  initialDispatch
} = connectRoutes(history, routesMap)

Then when you boot up your root saga you import {sagas as routeSagas} from ... and fork/spawn them as you see fit. Eliminates the extra mapping of sagas to types by bringing it directly into the RFR configuration.

Now what the sagas export actually looks like and how to handle that without importing too much, or anything, from redux-saga directly I don't know yet, haven't thought that far.

@faceyspacey
Copy link
Owner

faceyspacey commented Aug 15, 2017

import createHistory from 'history/createBrowserHistory'
import { connectRoutes } from 'redux-first-router'
import createSagaWithRouting from 'redux-first-router-saga'

import { ROUTE_HOME, ROUTE_ABOUT } from 'types'
import { homeSaga } from 'state/sagas/home'
import mainSaga from 'state/sagas'

const routesMap = {
  [ROUTE_HOME]: {
    url: '/',
    saga: homeSaga
  },
  [ROUTE_ABOUT]: '/about'
}

const history = createHistory()

export const {
  reducer,
  middleware,
  enhancer,
  sagas,
  initialDispatch
} = connectRoutes(history, routesMap, {
   mainSaga: createSagaWithRouting(mainSaga)
})

sagasMiddleware.run(sagas)

implementation:

redux-first-router/connectRoutes.js:

const connectRoutes = (history, routesMap, options) => {
   //...
   const routeTypeSelector = state => selectLocationState(state).type
   const sagas = options.mainSaga(routeTypeSelector, routesMap)
   //...

   return { reducer, enhancer, middleware, sagas }
}

redux-first-router-saga:

export default function createMainSaga(mainSaga) {
  return function mainSaga(routeTypeSelector, routesMap) {
    *function routesSaga () {
        const initialRoute = yield select(routeTypeSelector)

        if (routesMap[initialRoute]) {
           yield spawn(routesMap[initialRoute])
        }

        while (true) {
           const {type} = yield take(Object.keys(routesMap))
           yield spawn(routesMap[type])
        }
      }

      return *function sagas () {
        yield fork(routesSaga)
        yield fork(mainSaga)
     }
  }
}

@faceyspacey
Copy link
Owner

faceyspacey commented Aug 15, 2017

so bottom line is we make it a plugin package (of which we have several that function exactly like that is, i.e. as an option). That means we dont have to bundle anything that makes it work into RFR. and it does all the work for you, and lets you get back to your primary sagas work.

Essentially RFR has a budding plugin ecosystem that functions exactly like this.

  • restoreScroll -- ScrollRestoration functions like this (Its also another package)
  • navigators -- React Navigation support functions like this (Its also another package)
  • querySerializer -- for query params support (Its also another package)
  • callbacks are in the same boat (i.e. onBefore/AfterChange)
  • initialDispatch is similar
  • and now mainSaga or whatever it ends up being called. (Its also another package)

And as far as the amount of sagas code in RFR, it's the same as any other plugin: checks if it exists, and in 1-2 lines fires something on the plugin; then all the work is offloaded to the 3rd party package.

@bfillmer
Copy link
Author

@faceyspacey That looks pretty solid, if we can roll in a default initialDispatch whenever using the sagas option we simplify the routesSaga as well, and it follows that if you are using sagas you will need that initial dispatch to correctly fire off anything needed for the route the user is at on application load.

@faceyspacey
Copy link
Owner

I dont suppose u can show me how to do it. it's a blind spot of mine and I never fully understood the problem.

@faceyspacey
Copy link
Owner

faceyspacey commented Aug 15, 2017

I guess its just something like this:

export default function createMainSaga(mainSaga) {
  return function mainSaga(routeTypeSelector, routesMap, initialDispatch) {
    *function routesSaga () {
        yield put(initialDispatch()) 
        const initialRoute = yield select(routeTypeSelector)

        if (routesMap[initialRoute]) {
           yield spawn(routesMap[initialRoute])
        }

        while (true) {
           const {type} = yield take(Object.keys(routesMap))
           yield spawn(routesMap[type])
        }
      }

      return *function sagas () {
        yield fork(routesSaga)
        yield fork(mainSaga)
     }
  }
}

@faceyspacey
Copy link
Owner

but if it is that, then i dont see what the problem was, as u can just get it off the initialRoute. enlighten me.

@faceyspacey
Copy link
Owner

is it this now:

export default function createMainSaga(mainSaga) {
  return function mainSaga(routeTypeSelector, routesMap, initialDispatch) {
    *function routesSaga () {
        yield put(initialDispatch()) 

        while (true) {
           const {type} = yield take(Object.keys(routesMap))
           yield spawn(routesMap[type])
        }
      }

      return *function sagas () {
        yield fork(routesSaga)
        yield fork(mainSaga)
     }
  }
}

@faceyspacey
Copy link
Owner

that's less code, but i dont see how before without initialDispatch it was broken. that's what i'd like to get to the bottom of, just to know.

@faceyspacey
Copy link
Owner

faceyspacey commented Aug 15, 2017

Ooops, i get it, the problem with sagas before was simply so that they wouldn't have to do the initialRoute thing at all. Therefore my above implementation was wrong. It's this:

export default function createMainSaga(mainSaga) {
  return function mainSaga(routeTypeSelector, routesMap, initialDispatch) {
    *function routesSaga () {
        while (true) {
           const {type} = yield take(Object.keys(routesMap))
           yield spawn(routesMap[type].saga)
        }
      }

      return *function sagas () {
        yield fork(routesSaga)
        yield fork(mainSaga)
        yield put(initialDispatch()) 
     }
  }
}

@faceyspacey faceyspacey changed the title Improve experience with redux-saga Build a 3rd party plugin that optimizes Sagas usage (following this blueprint) Sep 7, 2017
@faceyspacey
Copy link
Owner

faceyspacey commented Sep 7, 2017

I've now labeled this "HELP WANTED." The blueprint above is pretty solid, but should be built and initially manually tested/used by someone who spends a lot of time with Sagas (which I no longer do). I'm happy to help. The above code basically needs to be peeled out into its own package, and then the RFR code that triggers it is ultimately very small (i.e. a few lines). It also would be nice if someone in the Sagas community made it and told others about it, which is part of the point of this. I don't wanna make the package and then nobody really knows about it--cuz i have no Sagas friends to tell :).

@faceyspacey faceyspacey changed the title Build a 3rd party plugin that optimizes Sagas usage (following this blueprint) Build 3rd party plugins that optimizes Sagas usage (following this blueprint) [and another for Redux-Observable] Sep 30, 2017
@faceyspacey faceyspacey changed the title Build 3rd party plugins that optimizes Sagas usage (following this blueprint) [and another for Redux-Observable] Build 3rd party plugins that optimize Sagas usage (following this blueprint) [and another for Redux-Observable] Sep 30, 2017
@klis87
Copy link

klis87 commented Oct 12, 2018

The question is whether actually there is any need to write integration with library like Redux-Saga. I personally use both RFR with Redux-Saga and I do stuffs like this:

function* fetchSaga() {
  yield put(fetchSth());
}

function* rootSaga() {
  yield takeLatest('SOME_ROUTE', fetchSaga);
}

Because RFR is first Redux citizen, not some wrapper or hacking integration library, we can really do whatever we want in our sagas, like react on route change, get current route with selectors etc, we can even do SSR with pure Redux in Redux-Saga way with END action.

@ScriptedAlchemy what do you think about it, do I miss sth and maybe an integration library might be worth doing? If needed, I might do some integration as RFR/Rudy plugin then

@ScriptedAlchemy
Copy link
Collaborator

@klis87 i believe the idea is to attempt to reduce the amount of work needed outside RFR.

At least this is the case for Rudy - which if you are looking at working on a plugin - i would encourage you do to it on Rudy rather than RFR - Rudy is the successor. https://github.com/respond-framework/rudy

In rudy, we are trying to create an api which allows for plugins and middleware to be built. Things like connectors or adaptors. The whole reason for these plugins is to reduce the amount of time and guesswork for other developers when trying to integrate. If theres a plugin, then we have a standardized way to instantiate it.

Its an important aspect of the vision - I see Rudy getting Vue middleware, graphql adapters, devtools plugins.

@gorhom
Copy link

gorhom commented Dec 5, 2018

@ScriptedAlchemy it would be great if this library could split into two libraries as well:

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

No branches or pull requests

7 participants