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

store provided to middleware doesn't implement Symbol.observable #1833

Closed
jayphelps opened this issue Jun 28, 2016 · 6 comments
Closed

store provided to middleware doesn't implement Symbol.observable #1833

jayphelps opened this issue Jun 28, 2016 · 6 comments

Comments

@jayphelps
Copy link
Contributor

https://github.com/reactjs/redux/blob/master/src/applyMiddleware.js#L25-L29

Because it isn't a real store, this can cause issues with middleware that need some of the real store's features. Particularly the one we care about right now is the Symbol.observable interop. We need the ability to receive a stream of state changes in redux-observable

I imagine the reason for providing a mock store is to not bleed abstractions like replaceReducer right? Any objections to including the Symbol.observable interop in that mock store too? (or just not mocking it)

@jayphelps
Copy link
Contributor Author

Didn't think I had time to PR but it was fairly easily #1834

jayphelps added a commit to jayphelps/redux that referenced this issue Jun 28, 2016
jayphelps added a commit to jayphelps/redux that referenced this issue Jun 28, 2016
@gaearon
Copy link
Contributor

gaearon commented Jun 29, 2016

Hmm. We actually don't provide this on purpose (just like we don't provide store.subscribe there). The original intention was that Redux middleware cannot subscribe to the store. Since it already receives dispatches it seemed like people would use this incorrectly and leak subscriptions.

Middleware always received just a subset of store API. Can you please show how you intend to use it in Redux Observable?

@gaearon
Copy link
Contributor

gaearon commented Jun 29, 2016

(We also don't mean it as a "mock" store—rather we meant this is a subset of store API that is ignorant of subscriptions. It will also literally become ignorant of them after #1702 so even if we add it now, we would probably break it later.)

@jayphelps
Copy link
Contributor Author

jayphelps commented Jun 29, 2016

@gaearon It's pretty trivial to recreate this at the middleware level

const middleware = processManager => store => {
  const action$ = new Subject();
  const state$ = new BehaviorSubject();

  processManager(action$, state$).subscribe(store.dispatch);

  return next => action => {
    const result = next(action);
    action$.next(action);
    state$.next(store.getState());
    return result;
  };
};

http://jsbin.com/nupaceb/edit?js,output

So far the true use cases for this have been relatively weak. Mostly around logging or replicating the state to some persistence layer like localStorage--being able to turn these things on/off/etc via actions. That said, I think the more common situation is allowing redux-observable users to write "pure" RxJS code instead of mixing reactive + imperative. Some people prefer the purity, even if it's more verbose. I'm not sure what my stance is on that, yet. I imagine most cases the imperative approach is more worth it in clarity..but I don't want to say "no" to purists.

// state value is looked up imperatively
 const incrementIfOddManager = (action$, store) =>
  action$.ofType(INCREMENT_IF_ODD)
    .filter(() => store.getState().counter % 2 === 0)
    .map(increment);

/* vs */

// more "pure" reactive (but this doesn't work yet cause it isn't real store)
const incrementIfOddManager = (action$, store) =>
  action$.ofType(INCREMENT_IF_ODD)
    .switchMap(action =>
      from(store).first()
        .filter(state => state.counter % 2 === 0)
        .map(increment)
    );

@gaearon
Copy link
Contributor

gaearon commented Jun 29, 2016

My intuition is that if you want to subscribe to the store, you want it to be an enhancer rather than a middleware.

@jayphelps
Copy link
Contributor Author

@gaearon that's definitely an option for us. If you have a moment to describe (or link me) what the concern against subscribing to the store inside middleware; i.e. "leaking" subscriptions implies anti-pattern but doesn't explain why it is one.

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

Successfully merging a pull request may close this issue.

2 participants