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

RFC: provide state$ as a stream argument #56

Closed
jayphelps opened this Issue Jun 27, 2016 · 38 comments

Comments

Projects
None yet
8 participants
@jayphelps
Member

jayphelps commented Jun 27, 2016

The store provided by redux (and given to Epics) isn't a full store, so it doesn't have the store[Symbol.observable]() interop point to support Observable.from(store). This is by their design.

Let's consider changing the Epic signature to function (action$: ActionsObservable, state$: BehaviorSubject<State>, store: Store), making a stream of stage changes the second argument and moving the store to the third argument.

const fetchUserEpic = (action$, state$) =>
  action$.ofType(FETCH_USER)
    .mergeMap(action =>
      getJson(`/users/${action.id}`, { 'Authorization': `Bearer ${state$.value.authToken}` })
        .map(respose => fetchUserFulfilled(response))
    );

// or the "reactive" way, but more verbose

const fetchUserEpic = (action$, state$) =>
  action$.ofType(FETCH_USER)
    .withLatestFrom(state$.pluck('authToken'))
    .mergeMap(([action, authToken]) =>
      getJson(`/users/${action.id}`, { 'Authorization': `Bearer ${authToken}` })
        .map(respose => fetchUserFulfilled(response))
    );
const localStorageStateEpic = (action$, state$) =>
  state$
    .filter(state => state.someState)
    .distinctUntilChanged() // state is supposed to be immutable, so this should be safe
    .throttleTime(500) // may or may not want to do something like this for perf reasons
    .do(state => localStorage.setItem('someState', JSON.stringify(state)))
    .ignoreElements(); // we have no actions to emit

I'd want to keep the store still, since while not necessarily idiomatic to use, is a handy escape hatch to "get shit done".

If this is a good idea in general, we'd need to decide what kind of observable the state changes were in. My initial thoughts were a BehaviorSubject so that the imperative value property was available for the same common cases people use store.getState() for or they can operate on it and it emit the last value right away. Because it's an observable, it feels slightly more idiomatic and then people don't need to learn about the fact that Redux's store supports the Symbol.observable interop so you could just do Observable.from(store), which isn't immediately obvious.

Obviously, this is not really all that different than existing solution..it's more whether we should provide a obvious "Rx way" or not. Please feel free to debate this.

Cc/ @blesh

@benlesh

This comment has been minimized.

Collaborator

benlesh commented Jun 27, 2016

The only thing that wouldn't be great about this is the fact that you'd remove the ability to dispatch from a do() block.

(action$, store) => action$.ofType('TWO_THINGS')
  .delay(1000)
  .map({ type: 'THING_ONE' })
  .do(store.dispatch)
  .delay(1000)
  .map({ type: 'THING_TWO' });

You'd almost want to add a third argument that was an observer that dispatched.

(action$, state$, dispatcher) => action$.ofType('TWO_THINGS')
  .delay(1000)
  .map({ type: 'THING_ONE' })
  .do(dispatcher)
  .delay(1000)
  .map({ type: 'THING_TWO' });
@benlesh

This comment has been minimized.

Collaborator

benlesh commented Jun 27, 2016

Similarly, it could just be the dispatch function itself... I'm not sure what dispatcher would do on error or complete, honestly.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 27, 2016

@blesh sorry if my original post wasn't clear. I'm proposing moving the store to the third argument, keeping it because it is indeed a handy escape hatch, even if it isn't always "idiomatic" per say.

@istarkov

This comment has been minimized.

Contributor

istarkov commented Jun 27, 2016

What the real need to access the state object (out of initialization step)?
Any real example.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 27, 2016

@istarkov I'm not sure what you're asking? What is the need to access the current store's state? My original post includes two examples.

@istarkov

This comment has been minimized.

Contributor

istarkov commented Jun 27, 2016

I mean state data not dispatch.

@istarkov

This comment has been minimized.

Contributor

istarkov commented Jun 27, 2016

In your examples

 action$.ofType(INCREMENT_IF_ODD)

does not look as a job which process should solve - just move that into the reducer.
As IMO actions should not be more than data deltas, than something that depends on real state.
(This is needed for optimistic updates etc)

So any real life example?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 27, 2016

@istarkov that one is a semi-real life example because some people don't agree with placing that sort of logic in the reducer. I don't personally have much of an opinion on it cause it's not something I've run into very often.

I've had to do things like this tho:

Observable.combineLatest(
    actions.ofType(SEARCH_VALUE_CHANGED)
      .debounceTime(250)
      .map(action => action.value)
      .startWith(getState().search.value)
      .filter(value => !!value),
    actions.ofType(SET_PAGE)
      .map(action => action.value)
      .startWith(getState().search.page),
    (q, page) => `https://api.github.com/search/repositories?q=${q}&page=${page}&per_page=${resultsPerPage}`
  )
  .switchMap(url => /* etc */);

But again, arguments can be made like "why is the search value in the store", etc.

@istarkov

This comment has been minimized.

Contributor

istarkov commented Jun 27, 2016

As I see examples is about that I wrote before,
that access to state is needed only on initialization step,
startWith(getState().search.value) but there is no need to get state changes after.

@alisd23

This comment has been minimized.

alisd23 commented Jun 27, 2016

@istarkov I think it's very common to do business logic in action creators (or managers) instead of in the reducers, as I see a reducer as only being a transformation of one state to the next.
Also in terms of debugging (when logging actions), generally it's nice to be able to see more specific actions, and prevent the firing of certain actions, depending on state, which makes the flow of actions clearer.

So being able to use the current state to conditionally dispatch actions for different situations in the managers makes sense to me. And I think the state$ stream is a good idea as it removes the robustness of getState()

@istarkov

This comment has been minimized.

Contributor

istarkov commented Jun 27, 2016

@alisd23 IMO having dependency of async process on a state, makes your code less predictable comparing with the process which depends on it's own state.

This also add an additional dependency, so every time you will change the state implementation you should check any possible changes in processes source.

Yes, tracking own process state could cause a code duplication and I think there could be cases there it will be not the best solution, but before adding additional stream, may be we should found that real case.
As in my practice having a lot of middlewares, I never read redux state in any of them.

@istarkov

This comment has been minimized.

Contributor

istarkov commented Jun 27, 2016

@matthewwithanm

This comment has been minimized.

matthewwithanm commented Jun 28, 2016

Sorry if this is a dumb question, but the store that Redux passes to the middleware doesn't actually implement Symbol.observable IINM. Is redux-observable doing something special to get it?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 28, 2016

EDIT: this comment is WRONG. Ignore it.

@matthewwithanm it actually does! 😄 http://jsbin.com/bipiji/edit?js,output we don't need any magic, just need to Observable.of(store) Observable.from(store).

It's just the standard store, not a mock or gimped one. https://github.com/redux-observable/redux-observable/blob/master/src/reduxObservable.js#L9

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 28, 2016

@matthewwithanm whoaaaaa apparently I stand corrected. They do give you a fake store https://github.com/reactjs/redux/blob/master/src/applyMiddleware.js#L25-L29

I'm not sure how this is working...will look into it hahahaha 😮

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 28, 2016

(sorry for the frequent comments....it's late...)

@matthewwithanm I'm blind. the example jsbin I gave does not log the store state out. Soooooo..this is a problem indeed. I'll bring it up to the redux team.

@matthewwithanm

This comment has been minimized.

matthewwithanm commented Jun 28, 2016

I think you want Observable.from, right? You're creating one containing the store, not its states. If you use that, it'll yell at you about the fake store not being observable. 😞

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 28, 2016

@matthewwithanm nope, we do want Observable.of(store), we want a stream of state changes, which the Symbol.observable interop provides and Observable.of will use, but as you correctly pointed out (and I dismissed) redux doesn't provide the real store to middleware so we don't give you the real store either. The mock store is missing that interop so it's not currently possible at all.

Yes! you use Observable.from(store) not Observable.of(store). (he reached out to me outside of GH). Sorry everyone for my blind insistence! 😞😳

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 28, 2016

I PR'd support here reduxjs/redux#1834 for Symbol.observable on the mock store provided to middleware. We'll see what they say.

@alisd23

This comment has been minimized.

alisd23 commented Jun 28, 2016

@istarkov Yeah that was the kind of logging I was talking about, I meant actually reading those action logs when there are less actions, and more specific actions makes following the logic of the app and finding bugs in state changes easier.
A useful example (a safety 'check' for the logout manager):

const logoutCoordinator = (action$, { getState }) =>
  action$
    .ofType(LOGOUT)
    .filter(action => Boolean(getState().auth.user))
    .map(logoutSuccess)
    .do(apiLogout);

In this case, although it is quite pedantic (as the LOGOUT action should not really be called from anywhere if the user is logged out), it is a useful check to see if the user is logged in before doing anything else. This ensures no api requests or state changes happen if the user is already logged out.

@alisd23

This comment has been minimized.

alisd23 commented Jun 28, 2016

Also I think @jayphelps first example with the incrementIfOddManager is similar; conditionally firing off actions depending on state. I've definitely seen that done before quite a lot - particularly with the thunk pattern.

@MrLoh

This comment has been minimized.

MrLoh commented Dec 13, 2016

Hmm, I really hope there will be a solution to this. It is really annoying to call store.getState() all the time and since the store is not even the real store object, you can't even create a workaround to it. I was trying to create a computer property state on the store Object.defineProperty(store, 'state', {get: () => store.getState()}), but even that isn't possible. Are you considering switching to a store enhancer as suggested in #100?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 13, 2016

It's been pretty low priority because in practice I haven't found it was useful to treat the store as a stream. I'd need more real world examples of where it would be useful.

@benlesh

This comment has been minimized.

Collaborator

benlesh commented Dec 13, 2016

There's another bit a nuance here. Generally the state will have just been updated by the time action$ emits a value. So if someone is doing something like action$.ofType('FOO').combineLatest(state$), they're likely to be executing code twice that wasn't necessary. In this situation, it would probably be better to pull the state out of the store imperatively... although, with proper guidance, action$.ofType('FOO').withLatestFrom(state$) would be ideal. I'm just more concerned with people screwing it up.

Regardless, they can accomplish the same thing right now just by using Observable.from(store).

@rgbkrk

This comment has been minimized.

Member

rgbkrk commented Dec 13, 2016

I'm confused... I thought (as outlined at the top of this issue) that the store is not a full store, it's missing the store[Symbol.observable]() interop.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 13, 2016

It is not a full store and does not support from(store) like the real store does. Ben is forgetting 😜

@MrLoh

This comment has been minimized.

MrLoh commented Dec 14, 2016

At least providing the current state as a parameter would be nice. I tried to write a wrapper for that, but it doesn't work because of the limited store. And if I do store.getState() in a custom createEpic function. I always just get the initial state.

@benlesh

This comment has been minimized.

Collaborator

benlesh commented Dec 15, 2016

Ben is forgetting

That's right! It's an object that duck-types as a pseudo-store.

@MrLoh

This comment has been minimized.

MrLoh commented Mar 5, 2017

With the dependencies option it should be pretty easy to inject the real store or the state stream of it into the epics now. Maybe that can be document as an option. I'll try it out later.

@MrLoh

This comment has been minimized.

MrLoh commented Mar 5, 2017

It works nice to inject the state$ Subject as a dependency:

let state$ = new BehaviorSubject(reducer())
let epicMiddleware = createEpicMiddleware(epic, {dependencies: {state$}})

let enhancer = applyMiddleware(epicMiddleware)
let store = createStore(reducer, enhancer)

store.subscribe(() => {
    state$.next(store.getState())
})

then in you epic, you can get it

let epic = (payload$, _, {state$}) => 
    payload$.withLatestFrom(state$).map(([payload, state]) => ...)

or

let epic = (payload$, _, {state$}) => 
    payload$.map((payload) => {
            let state = state$.value
            ...
    })

be aware that you can't call value on the state Subject anymore, after you applied an operator to it. So state$.filter(...).value won't work, because you are left with only an observable after applying the operator.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Nov 20, 2017

UPDATE

Still considering whether this one belongs. Leaning yes, but in the meantime you can make your own and inject it because you can use the action$ stream to get a stream of state updates--they are notified 1:1

const combineEpicsWithState = (...epics) => (action$, store, dependencies) => {
  // create a stream of state that is backed by a BehaviorSubject
  // so that new subscribers always receive the last state immediately
  // and have access to `state$.value`
  const state$ = new BehaviorSubject();
  action$.map(() => store.getState()).subscribe(state$);

  return combineEpics(...epics)(action$, state$, dependencies);
};

then your epics get have the signature

(action$, state$, dependencies) =>

No promises, but I'm guessing that'll likely be the signature of epics in 1.0.0 (again, no promises!)

const somethingEpic = (action$, state$) =>
  action$.ofType('SOMETHING')
    .switchMap(() =>
      state$.filter(state => state.anotherThing === 'something')
        .mapTo({ type: 'SOMETHING_READY' })
    );
@Sawtaytoes

This comment has been minimized.

Sawtaytoes commented Jan 10, 2018

After working with this for a while, I found using state$ directly required a lot of boilerplate. Since redux-observable is meant to get rid of that, I decided on passing a getState operator instead. This way, it doesn't map action and state props together unless the action type matches (performance reasons).

To ensure state works like a reducer, I put in a getter string that details the exact part of the state this epic should care about. Better code maintainability because it decreases coupling between the full Redux state and this little slice. It also make it so you don't have to dot through the state each time you wanna use it.

Example

addSomethingEpic.js

import { filter, map } from 'rxjs/operators'
import { ofType } from 'redux-observable'

import { ADD_SOMETHING, addSomething } from './actions'

const addSomethingEpic = (action$, addState) => (
  action$.pipe(
    ofType(ADD_SOMETHING),
    addState(), // Can optionally take a string so you can instead have a semantic `something.isLoading`.
    filter(({ state }) => state.isLoading),
    map(({ something }) => addSomething(something)),
  )
)

export default addSomethingEpic

redux-observable/combineEpicsWithState.js

import { combineEpics } from 'redux-observable'
import { map, switchMap } from 'rxjs/operators'

export const getStateFromStore = (store, stateBranch) => () => (
  stateBranch
  .split('.')
  .reduce(
    (state, branch) => state[branch],
    store.getState()
  )
)

const createStateAdder = state$ => (stateName = 'state') => (
  switchMap(action => (
    state$.pipe(
      map(state => ({
         ...action,
         [stateName]: state,
      }))
    )
  ))
)

const combineEpicsWithState = (
  (stateBranch, epics) => (
    (action$, store, dependencies) => {
      const state$ = (
        action$.pipe(
          map(getStateFromStore(store, stateBranch)),
        )
      )

      state$.subscribe({ error: console.error })

      return (
        combineEpics(...epics)(
          action$,
          createStateAdder(state$),
          dependencies,
        )
      )
    }
  )
)

export default combineEpicsWithState

rootEpic.js

import { combineEpics, combineEpicsWithState } from 'redux-observable'

import addSomethingEpic from './addSomethingEpic'
import decrementCountEpic from './decrementCountEpic'
import incrementCountEpic from './incrementCountEpic'
import removeSomethingEpic from './removeSomethingEpic'

export default combineEpics(
  combineEpicsWithState(
    'funPartOfTheState.importantStuff.somethings',
    [
      addSomethingEpic,
      removeSomethingEpic,
    ],
  ),
  decrementCountEpic,
  incrementCountEpic,
)
@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 10, 2018

Can you elaborate on it being a lot of work? If you mean subscribing or composing it in, you don’t have to as its a BehaviorSubject so the latest state is available at state$.value property. 🙂 the idea is best of both worlds; access to state as a stream or as just the single current value

@Sawtaytoes

This comment has been minimized.

Sawtaytoes commented Jan 10, 2018

I was unaware you could do state$.value (lacking in these docs: https://www.learnrxjs.io/). Sounds pretty awesome! Knowing that, I could instead do:

map(action => ({ ...action, semanticName: state$.value.funPartOfTheState.importantStuff.somethings }))

Since I'm assuming this would be common functionality, then it might be good to wrap it in a helper function. Maybe not. It's probable you'll use the action to pull something off the state then use that new value off the state to call another action creator.

My issue is having access to the entire state on the redux store. I'd like to limit it in my epic since I'm placing them in the directories with the reducers they're associated. From my perspective, they all go together and if an epic is in folder X, then it should only have access to the sub-state the reducers in folder X are using. This should, in theory, limit the maintenance costs of moving around files and make it easier for devs to know which epics go with with parts of the state based on the directory structure.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 10, 2018

My issue is having access to the entire state. I'd like to limit it in my epic since I'm placing them in the directories with the reducers they're associated. From my perspective, they all go together and if an epic is in folder X, then it should only have access to the sub-state folder X has.

I hear ya. I've felt that way sometimes, but at the same time it removes some of the reasons people choose redux--a single global atom of state, so its easy to share it without jumping through hoops. e.g. some epics that do an ajax request might need to look up its auth token or username, stored in redux.

It might be pretty interesting to consider whether having some sort of selector could be used to only provide the epic what it asks for. At this point I don't think it would be something that is required though since it seems like it would likely require extra boilerplate that many would find unnecessary since touching the store's state in your epic isn't super common; it happens, but I don't see a lot of it when doing things idiomatically.

jayphelps added a commit that referenced this issue Jan 24, 2018

feat(state$): The second argument of an Epic is now a stream of state…
…$, not a store. Closes #56

DEPRECATION: The second argument of an Epic is now a stream of
state$, not a store. You can access the current state imperatively on
the `state$.value` property, or by composing the StateSubject
reactively.

Learn more https://goo.gl/WWNYSP

jayphelps added a commit that referenced this issue Jan 24, 2018

feat(state$): The second argument of an Epic is now a stream of state…
…$, not a store. Closes #56

DEPRECATION: The second argument of an Epic is now a stream of
state$, not a store. You can access the current state imperatively on
the `state$.value` property, or by composing the StateSubject
reactively.

Learn more https://goo.gl/WWNYSP

jayphelps added a commit that referenced this issue Jan 24, 2018

feat(state$): The second argument of an Epic is now a stream of state…
…$, not a store. Closes #56

DEPRECATION: The second argument of an Epic is now a stream of
state$, not a store. You can access the current state imperatively on
the `state$.value` property, or by composing the StateSubject
reactively.

Learn more https://goo.gl/WWNYSP

jayphelps added a commit that referenced this issue Jan 24, 2018

feat(state$): The second argument of an Epic is now a stream of state…
…$, not a store. Closes #56

DEPRECATION: The second argument of an Epic is now a stream of
state$, not a store. You can access the current state imperatively on
the `state$.value` property, or by composing the StateSubject
reactively.

Learn more https://goo.gl/WWNYSP
@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 24, 2018

Hey everyone 👋 I've got a PR up with state$ at #410. Please take a look and let me know your thoughts. The API is generally the same as proposed except instead of a BehaviorSubject its a custom StateSubject; see ticket for explanation, but tl;dr you still can subscribe or use state$.value.

@rgbkrk

This comment has been minimized.

Member

rgbkrk commented Jan 24, 2018

nooooiiiiiccceeee

@Sawtaytoes

This comment has been minimized.

Sawtaytoes commented Feb 11, 2018

I noticed I was also able to use mergeMap or switchMapTo(state$) to grab the information I needed depending on the specific needs of that epic. Having store.getState changed to an observable has so many more uses!

jayphelps added a commit that referenced this issue Apr 4, 2018

feat(state$): The second argument of an Epic is now a stream of state…
…$, not a store. Closes #56

DEPRECATION: The second argument of an Epic is now a stream of
state$, not a store. You can access the current state imperatively on
the `state$.value` property, or by composing the StateSubject
reactively.

Learn more https://goo.gl/WWNYSP

jayphelps added a commit that referenced this issue Apr 4, 2018

feat(state$): The second argument of an Epic is now a stream of state…
…$, not a store (#410)

* feat(state$): The second argument of an Epic is now a stream of state$, not a store. Closes #56

DEPRECATION: The second argument of an Epic is now a stream of
state$, not a store. You can access the current state imperatively on
the `state$.value` property, or by composing the StateObservable (state$)
reactively.

Learn more https://redux-observable.js.org/MIGRATION.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment