ioof-holdings / redux-subspace

Build decoupled, componentized Redux apps with a single global store
https://ioof-holdings.github.io/redux-subspace/
BSD 3-Clause "New" or "Revised" License
312 stars 33 forks source link

[redux-subspace-observable] DEPRECATION message is trowed by redux-observable #65

Closed majo44 closed 6 years ago

majo44 commented 6 years ago

For such simple example:

import 'rxjs/add/operator/map';
import { compose, createStore } from 'redux';
import { createEpicMiddleware, subspaced } from 'redux-subspace-observable';
import { applyMiddleware,  } from 'redux-subspace';

let myEpic = (actions$) => actions$.ofType('A').map(a => {return { type: 'B'};});
let mySubspacedEpic = subspaced(state => state['nested'], 'nested')(myEpic);
let reducer = (state = {}) => {return state; };

let store = createStore(
    reducer,
    compose(applyMiddleware(createEpicMiddleware(mySubspacedEpic)))
);

store.dispatch({type: 'nested/A'});

the DEPRECATION message is trowed by 'redux-observable'

redux-observable | DEPRECATION: calling store.dispatch() directly in your Epics is deprecated and will be removed. Instead, emit actions through the Observable your Epic returns.
https://goo.gl/WWNYSP
    "redux": "^3.7.2",
    "redux-observable": "^0.17.0",
    "redux-subspace": "^2.1.0",
    "redux-subspace-observable": "^2.1.0",
    "rxjs": "^5.5.6",

Best regards.

P.S. Many thx for your effort on great lib :)

mpeyper commented 6 years ago

Hi @majo44,

Thanks for making us aware of this (we don't actively use redux-observable ourselves).

Looking into the issue, I think this is going to be quite annoying for us to work around, if we can at all.

The problem is that to correctly namespace the output actions, the best thing we can do is dispatch it into the subspace, which eventually will dispatch into the store and cause the deprecation warning to fire (or an undefined error in v1.0.0 as the warning suggests). To change it to namespace a plain action would be relatively easy to do, but I assume epics are able to emit thunks/promises/batched actions/anything else a redux middleware can handle and we need to ensure that whatever those end up dispatching also run through the subspace to get namespaced appropriately.

Unfortunately, I don't see an easy way around this and my experience with rxjs (and rx in general) is a bit lacking, so any help anyone can provide for this is much appreciated. I don't really want to stop supporting one of the big 3 side effect middleware (redux-thunk, redux-saga and redux-observable), especially if they are going to release v1.0.0 soon.

For anyone wanting to have a go at fixing it, the offending line is here and the redux-observable PR to change the functionality is here.

mpeyper commented 6 years ago

I just had a very interesting discussion with @evertbouw (the author of redux-observable/redux-observable#336) on the reactiflux discord channel for rxjs about this issue.

Basically, the dispatch function will be removed from the provided store in v1.0.0, but we discussed using the dependencies functionality to inject an unaltered store into the epics to be subspaced instead. Essentially the subspaced function would become:

const subspaced = (mapState, namespace) => {
  const subspaceDecorator = subspace(mapState, namespace)
  return epic => (action$, store, dependencies) => {
    const subspacedStore = subspaceDecorator(dependencies.store)

    const filteredAction$ = action$
      ::map((action) => subspacedStore.processAction(action, identity))
      ::filter(identity)

    epic(
      filteredAction$,
      { ...store, getState: subspacedStore.getState },
      { ...dependencies, store: subspacedStore }
    ).subscribe(subspacedStore.dispatch)

    return empty()
  }
}

and the createEpicMiddleware function would become:

export const createEpicMiddleware = (rootEpic, options = {}) =>
  applyToRoot(store =>
    baseCreateEpicMiddleware(rootEpic, { ...options, dependencies: { ...options.dependencies, store } })(store)
  )

This is essentially what I hacked together as a proof of concept and it worked, passing all the integration tests, but most of the unit tests fail (expectedly). There's a heap of safety code this could (should?) have as well, like what to do if options was passed something not an object, warning if dependencies did not have a store field when the epic was hit, etc.

At this point I think createEpicMiddleware should move out of the index.js file and into it's own file for unit testing, similar to our wrapper for redux-saga's createSagaMiddleware.

I'm more happy if someone else wants to take this on. I think the path is pretty straight forward from here and it won't properly be an issue until v1.0.0 or redux-observable is released, but I'd like to get this fixed up sooner rather than later (I know how annoying warnings can be, especially when there's not much you can do about it yourself).

evertbouw commented 6 years ago

I've followed your lead and also fixed the unit tests. I'll try to keep this library in mind next time I break things 😄

jayphelps commented 6 years ago

Excellent solution (injecting store) 👍

mpeyper commented 6 years ago

Fix was published in v2.2.0