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

An idea for integrating third party effect management libs such as redux loop/saga/observables #136

Closed
gniquil opened this issue Oct 4, 2017 · 15 comments

Comments

@gniquil
Copy link

gniquil commented Oct 4, 2017

TLDR:
Allow rfr to optionally dispatch ROUTE_CHANGE_BEGIN, ROUTE_CHANGE_RESOLVE, ROUTE_CHANGE_REJECT actions to replace thunk argument for routes.

Why:
All 3 libs (loop/saga/observables) relies on listening to store actions to trigger side effects (reducer, saga, epic respectively). So what we can do perhaps:

  • add a flag dispatchThunkActions (need better name), rfr will dispatch a ROUTE_CHANGE_BEGIN action.
  • loop/saga/observable can listen to this event, do the side effect thing
  • when the side effect resolves/rejects, loop/saga/observable will dispatch ROUTE_CHANGE_RESOLVE/REJECT
  • Once, rfr receives ROUTE_CHANGE_RESOLVE/REJECT, rfr will continue as usual.
@gniquil
Copy link
Author

gniquil commented Oct 4, 2017

Just to clarify, rfr doesn't dispatch _RESOLVE or _REJECT but only listens to them. And also the action types can follow rfr's style of generated from routesMap config ('/post' => POST => POST_RESOLVE/POST_REJECT)

@faceyspacey
Copy link
Owner

faceyspacey commented Oct 4, 2017

Cool.

Currently, the thunks all happen after the route changes and state updates. There are a few things like scroll restoration, page title change etc, that happen after, but essentially you'd only be telling RFR to do those things.

That said, I got a lot of new things coming that will provide a lot more callbacks (on both the route and global level), as well as an option to shift the order of the thunk to happen before route changes. In that setup, this would make more sense, but without that, the thunk is already occurring at the tail end of things, and ultimately that's where you would kick off observables and sagas currently. So there's nothing left to do--UNLESS THEY ARE KICKED OFF EARLIER.

on another note, Sagas has a pretty crappy SSR story. Your proposal could possibly simplify that, and I assume for observables too.

Otherwise, I'm not fully sure what the business goal is. Sagas and Observables can be kicked off pretty easily. Is the goal to insure other minor stuff like the page title changing happens? And in the correct order? Or is the goal to in fact delay the route change action until sagas/observables complete? In that case, it's not just about picking up when told by sagas/observables, but re-ordering things, and I happen to be working on stuff like that (literally, as we speak). Tell me more.

@gniquil
Copy link
Author

gniquil commented Oct 4, 2017

It's great to hear more hooks are coming! Thanks!

For me, the reason I'm bringing this up is we use redux-loop. And all of our side effects, data loading things are encapsulated in the reducers. It'd be nice to keep it that way, which includes route changes. In addition, as you alluded to in your last paragraph, sometime we do need to affect the order of things, and this ideally (in the context of redux-loop) would be happening in my reducers reacting to redux actions (and generally saga/observables are similar, just that they want to do it in the sagas or epics).

Nevertheless, as long as there are more hooks coming, and that I can replicate all the async behavior without the thunks, but purely based on actions dispatched from RFR (or use your upcoming hooks to dispatch the appropriate actions), it will be ok.

Thanks for the fast response. Excited to see what's coming.

@sachmata
Copy link

sachmata commented Oct 4, 2017

Take a look at https://github.com/ChaosGroup/redux-saga-first-router it is way simpler.

@faceyspacey
Copy link
Owner

@sachmata i'll check it out. Can you check this out too and let me know what you think:

#76 (comment)

@faceyspacey
Copy link
Owner

@gniquil still could you tell me more about the reverse, i.e. RFR listening to certain actions. I just want to make sure I have all bases covered. Are there any scenarios where that is just a killer must-have feature?

@sachmata
Copy link

sachmata commented Oct 4, 2017

Yep, sure! From first look I miss cancelling of spawn route sagas when new route is navigated. Other thing that I consider important is defining routesMap as actual Map that guaranties the order of iteration, using plain JS object is convenient but cannot be used to define route match priority. Cheers :)

@gniquil
Copy link
Author

gniquil commented Oct 5, 2017

@faceyspacey I guess what I was thinking more of an approach bottom up. For now, let's assume redux-first-router didn't exist, and I'd like to create a routing lib that works with all effect management libs through redux. What I'd do is to dispatch an action on every state change of routing, and pause and listen to a well defined set of actions from users to continue the routing flow.

For example, we have the following flow:

  1. user push new url to history
  2. load some data based on the new url
  3. resolve or reject based on loading result
  4. if success, change the url in the address bar, if fail, stay on the same page

Translating above to redux actions:

  1. user (of RFR) dispatch ROUTE_TO action
  2. RFR upon seeing ROUTE_TO, do some work based on config, and dispatch ROUTE_CHANGE_BEGIN
  3. user upon seeing ROUTE_CHANGE_BEGIN, start loading data, once done, dispatch ROUTE_CHANGE_CONTINUE or ROUTE_CHANGE_ABORT
  4. RFR upon seeing ROUTE_CHANGE_CONTINUE, change the url in the address bar, and dispatch ROUTE_CHANGE_COMPLETE

Note, ROUTE_TO, ROUTE_CHANGE_CONTINUE, ROUTE_CHANGE_ABORT are actions dispatched by the user, and ROUTE_CHANGE_BEGIN, ROUTE_CHANGE_COMPLETE are actions dispatched by the RFR.

If you structure it this way, async behavior is well defined without ever using thunk or promises. And for sideeffect management libs, it's clear how they can "plug" themselves into the flow. For e.g. in Saga (pseudo code... haven't used saga for a couple of months):

// as user of RFR, inside a saga
yield put(ROUTE_TO, newURL)
yield take(ROUTE_CHANGE_BEGIN)
// load data async
const result = // load data async
if (result.success) {
  yield put(ROUTE_CHANGE_CONTINE)
} else {
  yield put(ROUTE_CHANGE_ABORT)
}

With redux-loop, it's even more straight forward as it is just part of the standard reducer workflow.

The bottom line is if you dispatch actions on every "state change" of routing process, and wait for decisions via actions dispatched from your users, then it would be easier to integrate those libs. Note also that the above example is intentionally kept simple. In reality, the routing flow is probably much more complex (leaving a previous route, transition to a new one, prempted by something). But the method above should still work.

However, as you alluded to earlier, if you make more hooks available, certainly we can just dispatch those actions ourselves and is still fine.

@faceyspacey
Copy link
Owner

Excellent. I got the essence 100%. The challenge of that essence on my side is the listening of actions, specifically the CONTINUE action.

That's something not currently planned that I gotta think about more. Very interesting :)

@faceyspacey
Copy link
Owner

faceyspacey commented Oct 5, 2017

@sachmata taking from your code, this should do the trick for the cancel functionality, correct?:

function* onNavigate(routesMap, action) {
   const { type } = action
   const saga = routesMap[type] && routesMap[type].saga

   if (saga) {
      yield fork(saga, action) // will automatically terminate when parent terminates
   }
}

export default function createSagaWithRouting(mainSaga) {
  return function mainSaga(routesMap, dispatchFirstRoute) {
    function* routesSaga () {
         // takeLatest automatically cancels previous route
         yield takeLatest(Object.keys(routesMap), onNavigate, routesMap)
      }

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

this is the idea for a plugin

The other thing I'm concerned about is SSR. I've been tracking this thread:
redux-saga/redux-saga#255

And basically it seems SSR in the sagas world hasn't been a completely checked box. At least, at this point it's non obvious and not well documented. Perhaps RFR could provide some automation here.

To solve SSR with RFR + Sagas actually seems to go back to @gniquil 's original idea about having RFR listen to special actions such as CONTINUE, thereby giving specialized async middleware control of when the route truly changes (i.e. updates state with the route's type, and changes the address bar). So similarly, RFR needs to know when things are done (perhaps when a special action is dispatched, or perhaps it also uses CONTINUE so less is different between the server and client). Then the above "plugin" code if run on the server, would use this from the Sagas SSR API proposal:

store.dispatch(END)
      rootTask.done.then(() => {
        res.status(200).send(

to do this:

export default function createSagaWithRouting(mainSaga) {
  return function mainSaga(routesMap, dispatchFirstRoute) {
     function* routesSaga () {
         // takeLatest automatically cancels previous route
         yield takeLatest(Object.keys(routesMap), onNavigate, routesMap)
      }

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

        if(typeof window === 'undefined') {
            yield take('CONTINUE')
            store.dispatch(END) // not sure what the purist way to get `store` is here
        }
     }
  }
}

Then the way you put this together in configureStore.js is like this:

import createSagaWithRouting from 'redux-first-router-saga'

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

// createStore etc

const rootTask = sagasMiddleware.run(sagas)
await rootTask.done

const app = renderToString(<App store={store} />)
res.status(200).send(app)

So what do you think about listening to the same CONTINUE action for dispatching END on the server? Would this work? It seems RFR really could play a powerful role in the Sagas SSR story. Let me know how you'd rework this.

@faceyspacey
Copy link
Owner

@sachmata one thing about the ordering of object keys, basically they are ordered since none of the keys can be parsed as integers:

https://stackoverflow.com/a/23202095/213861
https://bugs.chromium.org/p/v8/issues/detail?id=164

All the major browsers have been doing this for a long time. It's just not in the spec. And I think that now has changed as of ES2015, as one guy in the above stackoverflow thread keeps pointing out. It is guaranteed based on insertion order, again at least for strings.

I remember checking this long ago when IE7 was still around and even there it was true. All the new automatically updated browsers guarantee this as well. I think for these small object literals we're good to go. For what it's worth, I had this conversation in another thread here and it wasn't able to be disproven. It's highly convenient that such is true, but if someone can disprove it, I'll add an order key on routes, and then if there are multiple matching routes, will take the route with the lower order number (that way you don't end up having to add order to every single route when it is only truly needed by a few groups).

@avionbg
Copy link

avionbg commented Oct 8, 2017

This works for me (shortened version without custom sagas, appReducer and other standard code...):

STORE :

/* /state/store.js */
import { applyMiddleware, compose, createStore } from 'redux'
import createSagaMiddleware from 'redux-saga'

import saga from '@/state/sagas/root' // Root saga
import {
  middleware as routesMiddleware,
  initialDispatch,
  routesMap
} from '@/state/routes'
import { appReducer } from '@/state/reducers'

const sagasMiddleware = createSagaMiddleware()

const composeMiddlewares = applyMiddleware(routesMiddleware, sagasMiddleware)

// Use Redux DevTools Extension if available and not in production.
const composeEnhancers =
  (process.env.NODE_ENV !== 'production' && window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__) || compose

export const store = createStore(
  appReducer,
  composeEnhancers(routesEnhancer, composeMiddlewares)
)

sagasMiddleware.run(saga(initialDispatch, routesMap))

ROUTES:

/* /state/routes.js */
import { connectRoutes } from 'redux-first-router'
import createHistory from 'history/createBrowserHistory'
import qs from 'qs' // Using this for parsing nested queries

import routesMap from '@/state/routesMap'

const history = createHistory()

const { reducer, middleware, enhancer, initialDispatch } = connectRoutes(history, routesMap, {
  querySerializer: qs
})

export { reducer, middleware, enhancer, initialDispatch, routesMap }

ROUTES-MAP:

/* /state/routesMap.js */
import { ROUTE_DASHBOARD, ROUTE_COMPONENTS } from '@/state/types/routes'
import { loadDashboard, loadComponents } from '@/state/sagas/routes'

export default routesMap = {
  [ROUTE_DASHBOARD]: {
    path: '/',
    saga: loadDashboard
  },
  [ROUTE_COMPONENTS]: {
    path: '/components',
    saga: loadComponents
  },

ROUTES-GENERIC-SAGA

/* /state/sagas/routes */
import { call, fork, takeLatest } from 'redux-saga/effects'

function* onNavigate(routesMap, action) {
   // We already filtered non valid routes
    yield fork(routesMap[action.type].saga, action)
}

export default function createSagaWithRouting(initialDispatch, routesMap) {
  // Filter out routes without sagas - this is saga generator function so it won't affect performance
  const routesWithSaga = Object.keys(routesMap).filter(route => routesMap[route].saga)
  function* routesSaga() {
    // takeLatest automatically cancels previous route
    yield takeLatest(routesWithSaga, onNavigate, routesMap)
  }

  return function* sagas() {
    yield fork(routesSaga)
    yield call(initialDispatch) // *** Here we need call not "put"
  }
}

For put you need an actual action not the function call as the redux-first-router exports it with initialDispatch. (Internally function calls dispatch). So we need call instead which just executes the function.

And finaly,

ROOT SAGA

/* /state/sagas/root.js */
import { all, fork } from 'redux-saga/effects'

import customSaga from '@/state/sagas/customSaga'
import createSagaWithRouting from '@/state/sagas/routes'

export default function(initialDispatch, routesMap) {
  return function* rootSaga() {
    yield all(
      [customSaga, createSagaWithRouting(initialDispatch, routesMap)].map(fork)
    )
  }
}

@faceyspacey
Copy link
Owner

@avionbg very nice:

.filter(route => routesMap[route].saga)

For your needs, how did you feel about @gniquil 's idea of giving full control to Sagas? Currently, there is no way to block route changes OR make the route change happen after data fetching (in thunks or sagas). But if RFR could wait on a specific action, then sagas could control the flow.

Ultimately, it's not the setup i'd go for, but I can see why people might want it, or perhaps want it for some routes. Basically, doing the route change after async data fetching is complete simplifies component rendering logic, as you don't have to worry about not having data and displaying spinners as much. It makes it very easy to implement a global modal spinner throughout your app for every page, and then let your components never worry about it. They can as a result just assume that if they're gonna render they have the data.

A caveat to this is on initial load of the app, you want to render something immediately, rather than wait for the async data fetching to complete. So you better design that global spinner to look good there too (a modal one might look stupid). Basically you have to structure your components so you render the layout, and a placeholder for everything else, with a spinner showing until that placeholder is ready. It's less fine-grained, and shows changes less soon to the user. I'm not in love with that approach as I prefer to show the most relevant changes to the UI immediately (at the expense of perhaps a minor bit more work across components), but I think having the option is valuable.

Anyway, I also thinking seeing the complete flow controlled by sagas (or observables or redux-loop) is also quite valuable. Perhaps more valuable. Power users are in love with these async middlewares, and it's therefore very important for them to get the power of RFR as a truly first-class setup. Now that said, such power of having RFR listen to Sagas/Observables/Loop is only really needed if you prefer the aforementioned order and control over route changes. But likely even more opportunities/possibilities would open up if RFR had the listening capability.

@avionbg
Copy link

avionbg commented Oct 9, 2017

I like the idea very much, and if had found the redux-saga-first-router before redux-first-router, I would probably go with it or at least try it. (I didn't go trough their code, but I think they have something like that). Since the project that I'm working on has gone too far at this point, switching is not an option. I'll probably test it and play with it at some point in the future, who knows. Anyway routing doesn't have to be complicated like with react router and clones, that's why I was attracted to rudy, and I must say everything looks and feels waaaaaay better than before (with R.router)
Apart of that, I'm convinced that sagas are the future, or at least they are here to stay .. which I can't say for sure for thunk etc.

More about that idea, having full control doesn't necessary mean that you will use all of it's power. Someone can still implement simple routing, but having an option to control the flow of the app is a powerful thing, and even if you don't need it initially there can always come a point where having it would be a great advantage.. better than inventing workarounds and hacks.

There are endless possibilities, and our only limit (and enemy) is our imagination.

BTW. I really like the way everything is mapped and can be accessed later .. it's something I was dreaming about earlier with react [redux] router.

@loganpowell
Copy link

Sorry for pinging on a closed issue, but I was hoping someone in here could help me with such an integration. I'm particularly interested in how to hook RFR to redux-logic, but I believe any guidance about general integration with other middleware would be very helpful!

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

No branches or pull requests

5 participants