Do the design of ActionsObservable and StateObservable make sense with RxJS 6 thinking?

[wldcordeiro]

I noticed that these observables are going to remain in existence (or exist in the case of StateObservable) but they kind of go counter to the new way of creating Observables in RxJS 6. The static class methods are all going away in favor of creation functions. So instead of exporting the ActionsObservable you change the documentation to show a user how to use rxjs's of and from? The ones on the ActionsObservable seem to be pass-throughs.

As for the StateObservable I can't say that's as straightforward but it'd be nice to have the creation method pattern for both be like using the creation functions from RxJS 6. I'm sorry if I'm unclear here. I'm trying to read through the sources to see if I'm understanding right and working on a wip branch to migrate to RxJS 6 and redux-observable 1 so I wanted to make sure I migrate my tests idiomatically. 😄

Which versions of redux-observable, and which browser and OS are affected by this issue? Did this work in previous versions of redux-observable?

1.0.0alpha2

[jayphelps]

👍 You’re definitely correct thanks for kicking off the discussion and helping out 🙂

I plan to remove ActionsObservable entirely, but StateObservable is needed because it differs in behavior than any other built-in Observable or Subject. It’s similar to BehaviorSubject except that it does not require an initial value. This is required because we setup your epics before the first action has been dispatched, so that you don’t miss it. If we used a BehaviorSubject and an epic subscribes to it on setup, it would receive undefined.

Having a creation function makes sense but I’ve struggling with naming since the simple names usually used might be confusing in this case. Any suggestions?

[wldcordeiro]

Heh the naming pain seems to be shared ReactiveX/rxjs#3514

To be honest so far I haven't run into an example of where I needed an actual StateObservable of my own in my code or test code that I've migrated to using RxJS 6 and 1.0.0a2. It seems like if you migrate to combining in the latest state for testing purposes you can get away with const state$ = of(objectResemblingStateYouNeed) the cases where you use .value are where it gets funkier and all the more reason to get people using the normal form. 😄

I'd say killing ActionsObservable seems like a good plan, with the redundancy. As for the names I'd have the of and from equivalents and lazy but stateObservableOf and stateObservableFrom seem good to me, a little long but clear. 🤷‍♂️

[insidewhy]

A ReplaySubject with a buffer length of 1 is equivalent to a BehaviourSubject with no initial value (although it lacks the .value property).

[evertbouw]

In #477 I have examples where I am creating my own StateObservable so that both state$.value and withLatestFrom(state$) will work, in addition to state$.getState(). I'm only creating it once in a helper function so I don't really need this to be easier.

[jayphelps]

@ohjames

A ReplaySubject with a buffer length of 1 is equivalent to a BehaviourSubject with no initial value (although it lacks the .value property).

Totally, though I'm pretty sold on keeping a .value field for convenience

[insidewhy]

Some say that the .value property encourages people to write unreactive code and it's usually good to encourage the replay subject but I suppose it's pragmatic to keep it value around. Its nonexistence can usually be circumvented via withLatestFrom and/or combineLatest.

[frankwallis]

I'm also interested to know how to mock up a StateObservable using the rxjs TestScheduler marble syntax and createHotObservable etc? I don't see an easy way to do this right now, redux-observable may need to provide helper functions for testing?

[evertbouw]

@frankwallis It's not too hard, take a look here for examples #477

I want to ship something with the library when I know it works for almost everyone and is as easy as it can be. So do let me know if you struggle with something!

[frankwallis]

@evertbouw many thanks, I have commented on the other thread

[jayphelps]

An update: the current plan is to remove ActionsObservable, rename StateObservable to ValueObservable and have it an Observable that simply mirrors a source while caching the previous value on a .value prop; BehaviorSubject actually could work now for state$ because of some changes that were made in redux v4, however I don't want to expose a real subject to epics because it gives them the footgun of being able to do naughty things like state$.next('lolbad'). However when writing unit tests I will likely recommend people use BehaviorSubject for ease.

I'm also thinking we'll expose at least one operator for selecting state, inspired by ngrx--I've been discussing things with them and there's a possibility of us sharing code between our projects via https://github.com/rxjs-community/redux-common

[luisnaranjo733]

Is the plan for these changes to be introduced in the stable v1? Or in a future major version?

[jayphelps]

Plans are a bit in flux. ActionsObservable is sticking around for now as many are using rxjs-compat and need ofType on the prototype. Curious why you ask? Is there a specific change you need?

[luisnaranjo733]

I started the process of migrating my team's codebase to v1 beta, and I was just a little bit worried after reading this issue that I may have done this prematurely if there are still major breaking changes planned in the initial v1 release.

Sounds like this will come later if it does come, which sounds good to me. Thanks :)

[jayphelps]

Right on! Sorry for the false alarm. I underestimated how popular the rxjs-compat layer usage would be.

[Pappa]

I've been using ActionsObservable in my tests for a little while, but recently found a problem in my tests once takeUntil was added to an epic. As the action$ stream being synchronous, takeUntil causes the subscription to never be made, due to a recent change in behaviour #3504.

Here's the test:

    it('should receive a websocket message', done => {
        const action$ = ActionsObservable.of(loginSuccessAction());
        const fromWebSocket = of({ payload: 'Hi!' });
        const expectedActions = [onSocketMessageAction('Hi!')];
        
        notificationsEpic(action$, {}, { fromWebSocket })
            .pipe(toArray())
            .subscribe(actions => {
                expect(actions).toEqual(expectedActions);
                done();
            })
    });

And the code under test:

export const notificationsEpic = (action$, state$, { fromWebSocket }) => action$.pipe(
    ofType(LOGIN_SUCCESS),
    switchMap(() => fromWebSocket()
        .pipe(
            map(({ payload }) => onSocketMessageAction(payload)),
            catchError(e => of(onSocketErrorAction(e))),
            takeUntil(action$.pipe(ofType(LOGOUT_SUCCESS)))
        )
    )
);

The code behaves as expected in the app, but not under test due the the synchronous ActionsObservable being used. I tried a variety of approaches creating the action$ stream with an async scheduler, but couldn't see any change in behaviour.

I could switch all my tests over to marble tests, but I was hoping to find a straightforward replacement for the ActionsObservable that plays nice with takeUntil. Any suggestions?

[jayphelps]

@Pappa you've actually found a bug in rxjs that was indeed introduced as part of rxjs#3504. That PR intended to only skip subscribing to the source if the notifier emits a value synchronously but instead skips subscribing if the notifier completes synchronously.

I've made a PR to fix this upstream: ReactiveX/rxjs#4039

Until this is fixed, the solution is to make each action emitted async by scheduling using the AsyncScheduler. In the case of ActionsObservable.of() it can be provided as the last argument.

If you're using redux-observable v1 and rxjs v6 you also no longer need to use ActionsObservable, you could just use regular Observable of() from import { of } from 'rxjs'

Here's a stackblitz: https://stackblitz.com/edit/redux-observable-v1-playground-ewvnvq?file=index.js

import { asyncScheduler } from 'rxjs';
// etc...
const action$ = of(loginSuccessAction(), asyncScheduler);

This works because the action$ stream won't synchronously complete then.

[Pappa]

Thanks Jay. Much appreciated.