Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Limited compatibility with redux-sagas #18

Closed
mpeyper opened this issue Dec 15, 2016 · 9 comments
Closed

Limited compatibility with redux-sagas #18

mpeyper opened this issue Dec 15, 2016 · 9 comments

Comments

@mpeyper
Copy link
Contributor

mpeyper commented Dec 15, 2016

As identified in #8, there is only partial compatibility with redux-sagas.

There appear to be no issues until the namespacing feature is used. The actions being dispatched by sagas are not passing through our wrapper and the namespaces are not being applied so the reducers are discarding them.

I have taken a look at the problem myself, but without much exposure to sagas before, I'm struggling to find a solution so any help would be much appreciated.

@amitkothari
Copy link
Contributor

amitkothari commented Jul 4, 2017

Hi Michael,

I have added a simple redux-saga example with namespacing feature.
https://github.com/amitkothari/redux-subspace

@mpeyper
Copy link
Contributor Author

mpeyper commented Jul 4, 2017

Hey Amit,

Thanks for contributing!

I think the files I want to be concentrating on are the component sagas and the app sagas, right?

Firstly, when I'm looking at this, I'm keeping subspace's key concepts in mind, so having the parent control the bounds of the subspace and the child being unaware of the parent

Ideally, the interface when integrating the component sagas would look something like:

import { sagas as componentSagas } from '../component'
import { all } from 'redux-saga/effects'
import { namespacedSagas } from 'redux-subspace'

export default function* sagas() {
  yield all([ 
    namespacedSagas(componentSagas, 'component1') , 
    namespacedSagas(componentSagas, 'component2') 
  ])
}

where the component exports unmodified sagas and subspace takes care of the rest. From my understanding (which is admittedly limited on sagas), the issue here is that we don't have a way of intercepting the actions before the takeEvery and after the put because they are imported and called directly (i.e. not passed in like dispatch is to a thunk).

Because of this, it may be necessary to bleed some of the subspace logic into the component. When doing so though, it should be in a way that the parent (app in this case) is still in control and so that the component has to know as little about subspace's implementation as possible. For example, if the app sagas did not pass through a namespace, you example would not work.

If the component sagas looked something like:

import { takeEvery } from 'redux-saga'
import { put } from 'redux-saga/effects'
import { appendNamespace } from 'redux-subspace'
import { READY_TO_CHANGE_VALUE, changeValue } from './actions'

const changeValueGenerator = (namespace) => {
  return function* changeValueGenerator() {
    yield put(appendNamespace(changeValue(), namespace))
  }  
}

export default function* sagas(namespace) {
  yield takeEvery(appendNamespace(actionType, namespace), changeValueGenerator(namespace))
}

where appendNamespace would accept a string or an object, and append namespace appropriately, or leave it unmodified of namespace was undefined, then the implementation logic of exactly how namespaces are applied is hidden, and it allows the parent to integrate with or without a namespace. I do see this as potentially becoming unruly, the more sagas are added, particularly if they get chained many layers deeps.

Another feature to be wary of is global actions which should not be namespaced when taking or putting, so providing a function that can take care of all of these intricacies will help a lot for component reuse.

Thoughts?

@mpeyper
Copy link
Contributor Author

mpeyper commented Jul 4, 2017

Since your comment sparked my interest in sagas again, I've been looking at the documentation a bit more closely and I noticed a different runSagas interface that does allow dispatch and getState to be provided.

Technically it runs them outside of redux, but I'm wondering if we can provide a saga wrapper that is run inside redux, and passes the filtered actions into the subscribe of the sagas that it is running externally with a subspaced dispatch and getState for them to use.

Does this sound crazy?

@mpeyper
Copy link
Contributor Author

mpeyper commented Jul 5, 2017

So following the external runSaga path has led to this.

I think there is some real potential in this solution.

@amitkothari
Copy link
Contributor

Looks good 👍

@mpeyper
Copy link
Contributor Author

mpeyper commented Jul 5, 2017

I thought I should probably explain the main parts of the spike.

Firstly, we need to wrap the top-most parent's sagas with withStore (in hindsight, provideStore might be more appropriate):

const sagaMiddleware = createSagaMiddleware()

const store = createStore(reducer, compose(
    applyMiddleware(sagaMiddleware)
  ))

sagaMiddleware.run(withStore(sagas, store))

This injects the redux store into that saga's context (i.e. any saga it spawns will have access to it's context). We need to do this to get access to the dispatch and getState functions so they can be wrapped later.

Then when combining the component's saga's into the app's sagas we need to wrap them in a subspace (similar to wrapping a component when mounting it):

export default function* sagas() {
  yield all([ 
    subspacedSagas(componentSagas, state => state.component1, 'component1'), 
    subspacedSagas(componentSagas, state => state.component2, 'component2'),
    subspacedSagas(componentSagas, state => state.component3)
  ])
}

This is where the magic happens. subspacedSagas returns a generator that takes any action and pushes it into an external saga. If it has been namespace, it filter's actions out that do not match (excluding global actions), removing the namespace on the way in. If it has not been namespaced, all actions get pushed in.

The mapState is required here to cater for any select effects.

It also replaces the store in the context with a subspaced store, so theoretically (I have to try this), it should work with nested subspaced sagas (i.e. if the component's sagas included a subspacedSagas). I've just double checked my commit and it's not a complete subspaced store that gets replaced, but it could easily be.


I've also been thinking about how to actually release these helpers. They have a explicit dependency on redux-saga so I don't want to just bundle it in with redux-subspace, as not everyone will want to use sagas. However, much of the implementation relies on the internals of redux-subspace. The way it creates the substore is almost identical to that of SubspaceProvider, but none of that is exposed through the public API.

My feeling is that I would actually like to split the library into three parts:

  • redux-subspace - the core of subspace
    • subspaceStore(store, mapState[, namespace]) - returns a new store with getState and dispatch subspacing the provided store
    • namespaced(namespace)(reducer) - Higher-Order Reducer (HOR?) that filters actions with the provided namespace that are allowed into the reducer
    • GlobalActions
      • register(actionType) - register the provided actionType as a global action
      • isGlobal(action) - check if the provided action is a global action
  • react-redux-subspace - react-redux compatible components
    • <SubspaceProvider mapState={...} [namespace="..."]> - wraps the contextual react-redux store in a subspace using subspaceStore
    • subspaced(mapState[, namespace])(Component) - a HOC that wraps the component in a SubspaceProvider
  • redux-subspace-sagas - Higher-Order Sagas (HOSs?) for subspacing sagas
    • provideStore(store)(saga) - injects the provided store into the saga's context
    • subspaced(mapState[, namespace])(saga) - runs the saga in a subspaced Saga environment using subspaceStore

This way, redux-subspace is no longer tied specifically to React and can be used with any project using redux.

Note: I have changed the interfaces on some of the Higher-Order-Blah parts of the API. They will still function the same as they do now, but they are more composable, which is nice.


Thoughts on any of the above? @vivian-farrell, I'd be interested in your take on the proposed library split.

@vivian-farrell
Copy link
Contributor

@mpeyper
Based on what we discussed the other day, I think the proposal to split into separate packages within the same repo (like React packages https://github.com/facebook/react/tree/master/packages) makes sense. This will allow the NPM modules to remain as thin as possible and not require users to install bloat they don't need. The React pattern also means you can still have a single src folder to keep everything together but use webpack to build a bundle specific for each package. Alternatively, you could not do any bundling and just distribute the whole codebase with every package and leave it up to the consumers bundler to deal with transpile and bundling. Either way would solve your issue keeping the package dependencies lean.

@mpeyper
Copy link
Contributor Author

mpeyper commented Aug 15, 2017

Sagas support has now been added in 2.0.1-alpha. Please try it out and let us know if there are any issues. I'll close this once the version is out of pre-release.

@mpeyper
Copy link
Contributor Author

mpeyper commented Sep 21, 2017

V2 has been released now so I'm closing this issue.

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

No branches or pull requests

3 participants