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

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

Merged
merged 2 commits into from Apr 4, 2018

Conversation

Projects
None yet
9 participants
@jayphelps
Member

jayphelps commented Jan 24, 2018

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.

As a transitionary period for v1.0.0 the StateSubject supports state$.getState() and state$.dispatch() and produces deprecation warnings.

TODO

  • Update the documentation
  • Update the type definitions
    - [ ] Update deprecation URL https://goo.gl/WWNYSP to point to a FAQ entry

Does anyone have any concerns?


This one is a long time coming. It took a while because this approach was piloted with several people, to iron out any kinks.

Note that unlike what was proposed in #56, this makes a custom StateSubject instead of using BehaviorSubject. This was done because BehaviorSubject expects an initial value when you create it new BehaviorSubject(initialValue) and will emit that initial value to anyone who subscribes. If your epic subscribed to state$ immediately on app-boot, it would have gotten undefined because when your epics are called, the reducers have not yet been given @@redux/INIT so there is no state yet. This was a major footgun I found for some, so StateSubject instead waits for the first value to be given to it, which happens after @@redux/INIT.

I also piloted this StateSubject approach and found at least twice people accidentally accessed state$.value immediately when your epics were first called; before @@redux/INIT. So I added a warning if this happens, since I cannot think of a case where this should be intentional. I feel this is actually somewhat related to #254, in which they want an epic to emit an action immediately on being called, before the redux store is ready. So I'm gonna try to solve those two problems together in a different PR. Most likely by waiting to call your root epic until right after @@redux/INIT.


Some contrived examples

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
StateSubject.prototype.getState = function () {
if (process.env.NODE_ENV !== 'production') {
require('./utils/console').deprecate('calling store.getState() in your Epics is deprecated and will be removed. The second argument to your Epic is now a stream of state$ (a StateSubject), instead of the store. To imperatively get the current state use state$.value instead of getState(). Alternatively, since it\'s now a stream you can compose and react to state changes.\n\n function <T, R, S, D>(action$: ActionsObservable<T>, state$: StateSubject<S>, dependencies?: D): Observable<R>\n\nLearn more: https://goo.gl/WWNYSP');

This comment has been minimized.

@jayphelps

jayphelps Jan 24, 2018

Member

The google URL https://goo.gl/WWNYSP will need to be updated to a new entry in the docs.

This comment has been minimized.

@jayphelps

jayphelps Jan 24, 2018

Member

Wordsmithing on this deprecation message is welcome!

This comment has been minimized.

@rgbkrk

rgbkrk Jan 24, 2018

Member

Should function <T, R, S, D> be function <T, S, D>?

This comment has been minimized.

@rgbkrk

rgbkrk Jan 24, 2018

Member

What would be the usual pattern if people were using getState() often inside of epics, usually at the start? combineLatest?

This comment has been minimized.

@ksaldana1

ksaldana1 Jan 24, 2018

@rgbkrk action$.ofType(Constant).withLatestFrom(state$) is the pattern you'd most often use. I think we should make sure to be detailed about best Rx practices here, because combineLatest is not going to be the behavior people want, and I think it's an easy trap to fall into. Most often, you want the action$ filter source observable to throttle any further down calculations--combineLatest will fire downstream observers when any of those observables fires (if I'm understanding correctly).

This comment has been minimized.

@ksaldana1

ksaldana1 Jan 24, 2018

Also I believe <T, R, S, D> is the correct typings for Epics. The R generic parameter represents possible epic "output" actions. That feature has been asked for in a few issues. The R generic parameter should probably be either before or after the D dependency generic parameter to allow for a good use of defaults:
<T, S, D = any, R = T>

Which will allow most people to ignore this specific feature.

This comment has been minimized.

@jayphelps

jayphelps Jan 24, 2018

Member

👍 I just added a couple contrived examples to the OP. I agree the docs will need some best practices. I imagine a large majority of people will just use state$.value and won't have a lot of need for state as a stream. But it's handy when you do need it.

This comment has been minimized.

@jayphelps

jayphelps Jan 24, 2018

Member

Should function <T, R, S, D> be function <T, S, D>?

@rgbkrk It's not decided, but @ksaldana1 is right about what I was thinking; the ability to specify different type for the output stream. That'll have to be a separate PR. For now I think I'll remove this type signature from the deprecation to unblock this PR.

The R generic parameter should probably be either before or after the D dependency generic parameter to allow for a good use of defaults:
<T, S, D = any, R = T>

yeah I've struggled with this one. R = T is technically wrong as epics shouldn't output everything they input as that would be infinite recursion. Currently I'm thinking that if someone wants to type things they need to go all-in and type both input and output, with no defaults other than implicit any default. That way we're not encouraging incorrect things like R = T which could confuse people or worse hide bugs. I'll tag you both in PR for the Epic type signature stuff so we can continue the debate and refinement. I'm very happy you both are giving your thoughts.

This comment has been minimized.

@cjol

cjol Feb 5, 2018

Contributor

R = T is technically wrong as epics shouldn't output everything they input as that would be infinite recursion.

This might have already been laid to rest, but I don't think that this is technically wrong. I've always seen T as my "universe" of action types. Anything from within that universe could come out of the action$ stream, so it's a good input type. Re-otputting everything from the input would be infinite recursion (and bad), but this type signature doesn't imply that. It's just saying that the output could have any type from within our universe. It's a bit of a loose type, and doesn't tell us much about what the epic does, but it's not wrong.

This comment has been minimized.

@jayphelps

jayphelps Feb 5, 2018

Member

It's a bit of a loose type, and doesn't tell us much about what the epic does, but it's not wrong.

Sorry, that is basically what I meant. 😄 "wrong" in a similar sense that this type signature is "wrong":

function example(): string | number {
  // always returns a number, so while it technically _could_ return a string it never will
  return 1;
}

So I'd just like to encourage people to have the return type of their epics be specifically narrow to only the actions it outputs.

StateSubject.prototype.dispatch = function (action) {
if (process.env.NODE_ENV !== 'production') {
require('./utils/console').deprecate('calling store.dispatch() directly in your Epics is deprecated and will be removed. The second argument to your Epic is now a stream of state$ (a StateSubject), instead of the store. Instead of calling store.dispatch() in your Epic, emit actions through the Observable your Epic returns\n\n function <T, R, S, D>(action$: ActionsObservable<T>, state$: StateSubject<S>, dependencies?: D): Observable<R>\n\nLearn more: https://goo.gl/WWNYSP');

This comment has been minimized.

@jayphelps

jayphelps Jan 24, 2018

Member

Wordsmithing on this deprecation message is welcome!

This comment has been minimized.

@rgbkrk

rgbkrk Jan 24, 2018

Member

Wasn't this deprecated earlier? This could be a nice time to go ahead and remove it for the 1.0.0 release.

This comment has been minimized.

@jayphelps

jayphelps Jan 24, 2018

Member

It was, but I'd like to keep it as deprecated until v1.0.0-alpha. I'd like to get one more release out pre-v1.0.0-alpha and I'd prefer not to have a release with store.getState() but not dispatch(). It's also a fairly fundamental change for some people who rely on it a lot, which I've seen. They may not have their semver locked down enough so I figured we should be nice and wait for v1. What do you think of that?

This comment has been minimized.

@bleathem

bleathem Jan 25, 2018

Removing the deprecations together makes good sense, reduces the frequency at which folks have to make code changes.

This comment has been minimized.

@rgbkrk

rgbkrk Jan 25, 2018

Member

Oh that's fine since this will be in prior to v1

get value() {
if (this._value === UNSET_VALUE) {
if (process.env.NODE_ENV !== 'production') {
require('./utils/console').warn('You accessed state$.value inside one of your Epics, before your reducers have run for the first time, so there is no state yet. You\'ll need to wait until after the first action (@@redux/INIT) is dispatched or by using state$ as an Observable.');

This comment has been minimized.

@jayphelps

jayphelps Jan 24, 2018

Member

Wordsmithing on this warning is welcome. It's a hard thing to describe. I'm referring to this:

const somethingEpic = (action$, state$) => {
  state$.value // <------ oops
  // etc...
};

But less obviously it often happens like this:

const somethingEpic = (action$, state$) => {
  return action$.ofType(PING)
    .mapTo({
      type: PONG,
      value:   state$.value // <------ oops
    });
};

Maybe this is a confusing enough issue that the warning should include both of these examples? Or perhaps it's enough to link to a FAQ which does?

This comment has been minimized.

@rgbkrk

rgbkrk Jan 24, 2018

Member

I think linking to the FAQ is a great idea!

Any way we could use template literals here so I can review it on multiple lines? 😉

This comment has been minimized.

@rgbkrk

rgbkrk Jan 24, 2018

Member

I've read the message over a couple times and it makes perfect sense to me.

This comment has been minimized.

@rgbkrk

rgbkrk Jan 24, 2018

Member

I suppose I'd write this in less of a "you" style:

One of the epics used state$.value before the reducers have run for the first time, so there is no state yet. The state$.value can't be used until after the first action (@@redux/INIT) is dispatched or by using state$ as an Observable.

constructor(store) {
super();
// If you're reading this, keep in mind that this is
// NOT part of the public API and will be removed!

This comment has been minimized.

@rgbkrk

rgbkrk Jan 24, 2018

Member

Could always hide it with a local symbol.

// Way above 
const ACTUAL_STORE = Symbol('Actual Store');

  // here in the constructor
  this[ACTUAL_STORE] = store;
  this[ACTUAL_VALUE] = UNSET_VALUE;

I do of course realize you can get at this property still using Object.getOwnPropertySymbols. It's a little more hidden though without having to make a closure around a class.

This comment has been minimized.

@jayphelps

jayphelps Jan 25, 2018

Member

Trying to avoid Symbols since they aren't in IE 11 and some older versions of the others..but tbh I don't even know if redux-observable works in IE 11. I may use others features that break stuff.

@mistyharsh

This comment has been minimized.

mistyharsh commented Jan 25, 2018

Thank you, @jayphelps for this PR. I have been long looking into a feasible solution for this including building my own Rx based redux store. Specifically, I was looking to address two concerns:

  1. Providing state as a stream within my epics
  2. Providing state as a stream within my components

As I see from this PR and #56, we have addressed the first issue. I am not very clear how it addresses the second concern. Would I still have to do state.getState() from my angular/vue.js components? Will state not be available as observable within components.

@karptonite

This comment has been minimized.

karptonite commented Jan 25, 2018

Hey, so I'm not involved in this project, but someone referred me here from the rxjs Gitter channel because I was looking for something that worked like BehaviorSubject, but didn't have to be initialized. But someone else in the channel suggested that ReplaySubject(1) might behave exactly that way, so I thought I'd suggest that to you folks instead of the custom StateSubject. Disclaimer: I have no idea if this will actually work.

@ksaldana1

This comment has been minimized.

ksaldana1 commented Jan 25, 2018

@karptonite Thanks for the suggestion! Rx.ReplaySubject(1) and this custom StateSubject are incredibly close in functionality--with an important nuance that I think matters here.

This isn't my PR, so I don't mean to speak for Jay here, but I think the most important feature for StateSubject in this context is the access to the value() method. This will allow consumers of this library to use state$.value() imperatively without having to fully embrace Rx everywhere. Although I personally prefer going "all-in" with the operators, I think it's important for the accessibility of this library. A great deal of issues that come up are already Rx questions vs. library specific, and I think forcing the use of some of the less common Rx operators to do a common task (access the current value of state) will only exacerbate that.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 25, 2018

I think the most important feature for StateSubject in this context is the access to the value() method. This will allow consumers of this library to use state$.value() imperatively without having to fully embrace Rx everywhere.

Yep. I also think there's a medium probability we'll be adding custom operators on it for selecting state or doing other common things that might come up. ReplaySubject also was mostly intended for an n greater than one so it comes with some extra internal logic to handle that and since it'll be in the hot path (every state update) I'd feel guilty with the wasted cycles.

@karptonite super appreciate your input and suggestions though. Please continue.

@ksaldana1 btw you mentioned state$.value() but I want to clarify that its just state$.value because its a getter. 😄 It's only a getter for the warning stuff. Which I'm hopeful will be removed if we wait to set up epics until after the first initial state exists.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jan 25, 2018

@mistyharsh

  1. Providing state as a stream within my components

As I see from this PR and #56, we have addressed the first issue. I am not very clear how it addresses the second concern. Would I still have to do state.getState() from my angular/vue.js components? Will state not be available as observable within components.

redux-observable currently doesn't take make any opinion on your UI, or even the fact that a UI exists (e.g. it's being used by at least one person I know on the serverside unrelated to UI stuff)

But if you'd like the stream of state as a stream, and you have access to the real store, you can do this: const state$ = Observable.from(store). This works with the real redux store because it supports Symbol.observable, but it doesn't work with the "store" given to middleware because it's not a real store.

@mistyharsh

This comment has been minimized.

mistyharsh commented Jan 27, 2018

Thank you @jayphelps, that clears up my confusion. Waiting to get this PR merged!!!

@rgbkrk

rgbkrk approved these changes Feb 5, 2018

@baptistelemoine

This comment has been minimized.

baptistelemoine commented Feb 16, 2018

@jayphelps do you have any release date about this feature? for me it will completely change (in a good way) how we reason about that middleware

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 jayphelps force-pushed the state$ branch 3 times, most recently from 818aa6b to 4957ec2 Apr 4, 2018

@jayphelps jayphelps force-pushed the state$ branch from 4957ec2 to 7883f86 Apr 4, 2018

@jayphelps jayphelps merged commit 2ff3f6e into master Apr 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jayphelps

This comment has been minimized.

Member

jayphelps commented Apr 4, 2018

@baptistelemoine

This comment has been minimized.

baptistelemoine commented Apr 4, 2018

Yeah!

@jayphelps

This comment has been minimized.

Member

jayphelps commented Apr 4, 2018

Released as 1.0.0-alpha.0, which you need to explicitly install and now requires rxjs v6

npm install redux-observable@1.0.0-alpha.0 --save-exact
@gustavomick

This comment has been minimized.

gustavomick commented Apr 11, 2018

fyi after this webpack is returning me Module not found: Error: Can't resolve 'rxjs/operators'

@baptistelemoine

This comment has been minimized.

baptistelemoine commented Apr 11, 2018

you should have a look here : https://redux-observable.js.org/MIGRATION.html, this version requires rx6, with the use of pipeable operators

@gustavomick

This comment has been minimized.

gustavomick commented Apr 11, 2018

@baptistelemoine conceptual doubt related to this change, with this new improvement, is there any way to update state inside epic (would be a viable pattern?) , appreciate your thoughts thanks

@baptistelemoine

This comment has been minimized.

baptistelemoine commented Apr 11, 2018

This feature doesn't change how epics actually works, you always catch an action, make something async, or not, and return a new action which results in a state update, or not.
state$ will just add a way to combine actions changes with state changes in your epics. For example, imagine an AUTO_SAVE action combined with redux-form state changes that trigger an api request.

@gustavomick

This comment has been minimized.

gustavomick commented Apr 11, 2018

@baptistelemoine thanks, appreciate that info, trying to learn .. could you explain me why is not possible to update state in between in/out action streams, i mean it could save a lot of effort of having additional reducers, why that pattern is not a viable one? thanks

@baptistelemoine

This comment has been minimized.

baptistelemoine commented Apr 11, 2018

this is how the redux flow actually works, you never mutates state directly but dispatch actions instead, epics are just middleware in this flow.

@gustavomick

This comment has been minimized.

gustavomick commented Apr 11, 2018

@baptistelemoine I what trying to ask .. is why would not be a good practice to have a reducer as part of the epic transformation? that would save a lot of convoluted calls and dispersed biz logic. which are the design drawbacks? thanks in advance

ps. in fact one could do such a thing, in a super dirty way, adding a generic custom action type that inside a reducer, who updates the state that is saved from the action payload. and then any epic could return that type and state payload. super dirty but could do that job.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Apr 11, 2018

@gustavomick One could experiment with that but currently I'd like for redux-observable to stick to traditional redux side effects middleware. What you're describing seems conceptually similar overall to something like redux-loop and the elm architecture.

which are the design drawbacks?

The creators of redux did it this way intentionally this way and so it is not intended as design drawback but rather a strength. Separation of your concerns. Certainly people can disagree, but it's important to realize it was intentional, not an oversight.

If it sounds like I'm confused or you otherwise have a convincing argument I definitely encourage you to submit an RFC that outlines what you propose with specific examples of pros/cons. 👍

@gustavomick

This comment has been minimized.

gustavomick commented Apr 12, 2018

Fyi not trying to debate about pros/cons but as I said before is only about learning from people deeply involved.

@cjol

This comment has been minimized.

Contributor

cjol commented Apr 12, 2018

@gustavomick

This comment has been minimized.

gustavomick commented Apr 12, 2018

@cjol great ans yeah those are the insight I was looking, thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment