redux-observable / redux-observable

RxJS middleware for action side effects in Redux using "Epics"
https://redux-observable.js.org
MIT License
7.85k stars 466 forks source link

Effects as data #76

Open shuhei opened 8 years ago

shuhei commented 8 years ago

Do you want to request a feature or report a bug?
feature

What is the current behavior? I really like the idea of background epics because it provides an intuitive way to handle multiple actions like debouncing with the powerful operators of Rx. However, tests for epics require mocking HTTP requests or other side effects.

To address the same problem, redux-saga allows sagas to yield effects as data, such as call, executes the effects, and feed the results to sagas. This keeps sagas side-effect-free and testable without mocking.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsbin.com or similar. It's not a bug.

What is the expected behavior?

To employ the same approach to redux-observable, the straight forward way would be express effects as actions. We can have a dedicated middleware to receive side effect requests, execute them, and dispatches results as actions so that we can throw out side effects of epics.

import { call } from 'redux-observable/effects';

function epic(action$, store) {
  const fooReq$ = action$.ofType('FOO')
    .map(action => call('FOO_REQ', webapi.getFoo, action.payload.id));
  const foo$ = action$.ofType('FOO_REQ')
    .map(foo => ({ type: 'FOO_FETCHED', payload: foo }));

  return Observable.merge(
    fooReq$,
    foo$
  );
}

The example above should work but it's tedious to have pairs of request observable and response observable. I want to write the pair as a single Observable. A possible solution would be creating an operator that emits effect requests to a Subject and returns an Observable that emits effect responses. Let's call it as effect operator. Here I assumed that effect returns an Observable of Observables in order to express effect's result as an Observable.

import { call } from 'redux-observable/effects';

function epic(action$, store) {
  const effect$ = new Subject();

  const foo$ = action$.ofType('FOO')
    .effect(action$, effect$, action => call(webapi.getFoo, action.payload.id))
    .switch()
    .map(foo => ({ type: 'FOO_FETCHED', payload: foo }));

  return Observable.merge(
    effect$,
    foo$
  );
}

I wrote quick sketches for the effect middleware and operator.

``` js const EFFECT_REQUEST = Symbol('EFFECT_REQUEST'); const EFFECT_RESPONSE = Symbol('EFFECT_RESPONSE'); const effectMiddleware = store => next => action => { if (typeof action[EFFECT_REQUEST] !== 'object') { return next(action); } const { id, payload } = action[EFFECT_REQUEST]; const { type, args } = payload; switch (type) { case 'call': try { const returnValue = args[0].apply(null, args.slice(1)); if (isObservable(returnValue)) { next({ [EFFECT_RESPONSE]: { id, type, payload: returnValue } }); } else if (typeof returnValue.then === 'function') { returnValue .then(result => next({ [EFFECT_RESPONSE]: { id, type, payload: Observable.of(result) } })) .catch(error => next({ [EFFECT_RESPONSE]: { id, type, payload: Observable.throw(error) } })); } else { next({ [EFFECT_RESPONSE]: { id, type, payload: Observable.of(returnValue) } }); } catch (error) { next({ [EFFECT_RESPONSE]: { id, type, payload: Observable.throw(error) } }); } default: // TODO: Handle other effect types. return next(action); } }; ``` ``` js function effectOperator(action$, effect$, callback) { return Observable.create(subscriber => { const id = uuid(); const source = this; const responseSub = action$ .filter(a => a[EFFECT_RESPONSE] && a[EFFECT_RESPONSE].id === id) .map(a => a[EFFECT_RESPONSE].payload) .subscribe(subscriber); const requestSub = source.subscribe( value => { try { effect$.next({ [EFFECT_REQUEST]: { id, payload: callback(value) } }); } catch (e) { subscriber.error(e); } }, err => subscriber.error(err), () => subscriber.complete() ); // https://github.com/ReactiveX/rxjs/issues/1583 return new Subscription(() => { [responseSub, requestSub].forEach(s => s.unsubscribe()); }); }); } Observable.prototype.effect = effectOperator; ```

What do you think about this idea?

There are still some things to consider:

Which versions of redux-observable, and which browser and OS are affected by this issue? Did this work in previous versions of redux-observable? It's not a bug.

jayphelps commented 7 years ago

This is still pretty novel to me so I'm trying to fully understand the merit. Can you provide a rough example of how you would then test your Epics? Would you still test your API logic independently?

I tried looking at some redux-saga examples for insight, but their testing example leaves much to be desired for me https://github.com/yelouafi/redux-saga/blob/master/examples/shopping-cart/test/sagas.js. It appears to actually invoke the API--which I'm very much not used to. Is that a goal you're looking for? What if your API requires auth or is otherwise nondeterministic? I feel dirty hitting a real API in my tests but perhaps this is the new rage I haven't heard?

jayphelps commented 7 years ago

I also presume most people will use Rx to make their API calls regardless, so they can have good cancellation. In that case, is this much different than just extracting your API calls and invoking them directly? Wouldn't you otherwise lose cancellation with call()?

where-ever/api.js
export const fetchFoo = id => ajax.getJSON(`/api/foo/${id}`);
where-ever/foo.js
const fetchFooEpic = action$ =>
  action$.ofType('FETCH_FOO')
    .switchMap(action =>
      api.fetchFoo(action.payload.id)
        .map(payload => ({ type: 'FETCH_FOO_FULFILLED', payload }))
    );

The call(...) stuff appears to give you one more level of abstraction, but I can't figure out what that buys you if you're going to invoke the real API anyway. I'm very likely missing something, so please be patient with me. This is a very very interesting discussion! ๐Ÿ˜„

jayphelps commented 7 years ago

I'll be using verbiage that may sound dismissive, but trust that I'm not being that way but more being devil's advocate to make sure this stands up to criticisms I have at face value. ๐Ÿ‘

I found this document and it's explanation is more helpful https://github.com/yelouafi/redux-saga/blob/afba0d1cfcad994315785f63a720b7e777bf3932/docs/basics/DeclarativeEffects.md So you test that it will invoke the API correctly, without actually hitting it. My next question would be that if you also merge in the mapping of the response to the output action (to fulfill the request), would your tests mock the call() to give your Epic a fake API response? In that case..I'm still struggling to see it's better. You're now just mocking something else.


Seems in that case you'd want to extract your response logic into its own Epic, but that seems like a lot of boilerplate just so you don't have to mock the API. Maybe that's worth it to some?

const fetchFooEpic = action$ =>
  action$.ofType('FETCH_FOO')
    .switchMap(action =>
      call('FETCH_FOO_RESPONSE', api.fetchFoo, action.payload.id)
    );
const fetchFooResponseEpic = action$ =>
  action$.ofType('FETCH_FOO_RESPONSE')
    .map(response => ({ type: 'FETCH_FOO_FULFILLED', payload: response.someProperty.anotherProperty }));

Maybe the advisement is that if your response needs massaging you do it in your actual API fetching code, and if you do that, you do not need a second epic to map the response. Instead you can just handle it in your reducer?


I'm still not sure about cancellation..but I'm thinking perhaps call() returns an observable that can be cancelled, and if you cancel it, it cancels the underlying API request too? That seems on the right track..but then it's not clear how it makes the API when running in your app but does not do so in tests.


This might be doable with something like this: TOTALLY UNTESTED

// Emits the call action, then makes the request.
// if you .take(1), the request will never happen
const call = (type, fn, ...args) =>
  concat(
    of({ type: '@@EPIC_CALL', payload: { type, fn, args } }),
    fn(...args)
      .map(payload => ({ type, payload }))
  );

// your epic...
// the AJAX call can be cancelled by the app dispatching "FETCH_FOO_CANCELLED"
// and it also has error handling
const fetchFooEpic = action$ =>
  action$.ofType('FETCH_FOO')
    .switchMap(action =>
      call('FETCH_FOO_FULFILLED', api.fetchFoo, action.payload.id)
        .takeUntil(action$.ofType('FETCH_FOO_CANCELLED'))
        .catch(error => of({
          type: 'FETCH_FOO_REJECTED',
          payload: error.xhr.response,
          error: true
        }))
    );

/* TESTING */

const action$ = new ActionsObservable(
  of({ type: 'FETCH_FOO', payload: { id: 123 }})
);

// some hypthetical testing helper `emitsEqual()`

// We .take(1) both the epic and the call, so the request isn't actually made
emitsEqual(
  fetchFooEpic(action$).take(1),
  call('FETCH_FOO_FULFILLED', api.fetchFoo, 123).take(1),
  'fetchFooEpic should emit an Effect call(api.fetchFoo, 123)'
);
jayphelps commented 7 years ago

OKAY. I tested out my ideas and it seems to work as I expect. Using the .take(1) (described in previous comment), you can prevent the actual request from being made. Will wait to hear your thoughts. I'm probably completely off base on a weird tangent compared to your desire haha

Here's the demo: http://jsbin.com/melilut/edit?js,output

I'm still not sure how you would then test the rest of your code without mocking. e.g. the cancellation, catch error handling, etc.

jayphelps commented 7 years ago

To test the rest of it, it seems you'd need some way to mock the API calling itself. If you didn't mock the XMLHttpRequest, you'd need to mock some handler....

Demo: http://jsbin.com/gafuru/edit?js,output this quick example is brittle because it technically performs async things like delay(0) but you'd still see the errors in the console (try changing the tests to fail)

jayphelps commented 7 years ago

I totes forgot that redux actions should always be serializable and functions are not, so I'm back to the drawing board.

Let me know when you have more insight as you're clearly more familiar with this technique! ๐Ÿ™Œ๐Ÿผ

jayphelps commented 7 years ago

No joke just had an idea while falling asleep so jotting it down here on my phone. We could provide a call function third arg that just invokes the params provided regularly but when you test you just provide a different one to check what they called with, and then return an observable with fake response or error.

const myEpic = (action$, store, { call }) =>
  action$.ofType(FETCH_USER)
    .mergeMap(action =>
      call(api.fetchUser, 123)
        .takeUntil()
        .map(fetchUserFulfilled)
        .catch(error => of(...))
  );

// when testing, provide them yourself
myEpic(action$, store, { call: (fn, ...args) => {
  assert things and return response/error
} });

Will format and elaborate on this tomorrow but was afraid I'd forgot cause I'm half a sleep. Cheers.

shuhei commented 7 years ago

@jayphelps Thanks a lot for your comments and thinking about this! Sorry for this late reply. I've been busy today and have to go to bed soon...

As you mentioned, it's about declarative effects and invoking them outside of epics. It's just a dirty sketch but we will be able to test it like the following. I omitted mechanics to match request and response here to simplify it.

it('fetches foo', () => {
  // Let's say we have a Subject with ofType.
  const input$ = new ActionsSubject();
  const store = {};
  const outputs = [];
  fetchFooEpic(input$, store).subscribe(action => outputs.push(action));

  input$.next({ type: 'FETCH_FOO', payload: { id: 123 });
  expect(outputs[0].payload).toEqual(call(webapi.getFoo, 123));

  const payload = { id: 123, name: 'foo' };
  // TODO: We need something like UUID to match request and response.
  input$.next(callResponse(outputs[0].id, payload));
  expect(outputs[1]).toEqual({ type: 'FETCH_FOO_FULFILLED', payload });
  expect(outputs.length).toEqual(2);
});

it('cancels request', () => {
  const input$ = new ActionsSubject();
  const store = {};
  const outputs = [];
  fetchFooEpic(input$, store).subscribe(action => outputs.push(action));

  input$.next({ type: 'FETCH_FOO', payload: { id: 123 });
  expect(outputs[0]).toEqual(call(webapi.getFoo, 123));
  input$.next({ type: 'FETCH_FOO_CANCELED' });
  const payload = { id: 123, name: 'foo' };
  input$.next(callResponse(outputs[0].id, payload));
  // The response is ignored here.
  expect(outputs.length).toEqual(1);
});

Also, we'll be able to feed an error to the Subject and test error handling with an operator like the Details section of this issue.

I'll try to answer your other questions tomorrow.

jayphelps commented 7 years ago

Expanding on my last comment, I think there's a trivial solution without us making a single addition to the library. Simple dependency injection with default params.

// when running app, the third arg will not be provided so it will use `indirect.call`
// but in testing we can just provide that third arg with our own!
(action$, store, call = indirect.call) => { ... }

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

const indirect = {
  call: (fn, ...args) => fn(...args)
};

const api = {
  fetchFoo: id => ajax.getJSON(`/api/foo/${id}`)
};

const fetchFooEpic = (action$, store, call = indirect.call) =>
  action$.ofType('FETCH_FOO')
    .switchMap(action =>
      call(api.fetchFoo, action.payload.id)
        .takeUntil(action$.ofType('FETCH_FOO_CANCELLED'))
        .map(payload => ({ type: 'FETCH_FOO_FULFILLED', payload }))
        .catch(error => of({
          type: 'FETCH_FOO_REJECTED',
          payload: error.xhr.response,
          error: true
        }))
    );

// when testing...
const call = sinon.stub().returns(
  Observable.of({ id: 123, name: 'Bilbo' })
);
fetchFooEpic(action$, store, call).subscribe(...);

I'm pretty sure this is still technically a "mock" however, but I don't see any other way around it because we cannot dispatch actions which contains function references (they're not serializable).


FWIW we're still trying to figure out the best way to test Epics in general. RxJS v5 comes with some incredibly handy test helpers with TestScheduler.

Here's a full example using those helpers: http://jsbin.com/higeyur/edit?js,output

and tl;dr

it('handles cancellation correctly', () => {
  const expected = '---|';
  const hotAction$ = hot('a-b|', {
    a: { type: 'FETCH_FOO', payload: { id: 123 } },
    b: { type: 'FETCH_FOO_CANCELLED' }
  });
  const action$ = new ActionsObservable(hotAction$);
  const responseSubs = '^!';
  const response$ = cold('--a|', { a: { type: 'SHOULD_NOT_EMIT_THIS' } });
  const store = {};
  const call = createCall(response$);

  const test$ = fetchFooEpic(action$, store, call);
  expectObservable(test$).toBe(expected);
  testScheduler.flush();

  expect(call.calledOnce).to.be.true;
  expect(call.calledWithExactly(api.fetchFoo, 123)).to.be.true;

  expectSubscriptions(response$.subscriptions).toBe(responseSubs);
});

Since that's a decent amount of boilerplate, one could DRY it up..if they want to..

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

const expectEpic = (epic, { expected, action, response, callArgs, store }) => {
  const testScheduler = new TestScheduler((actual, expected) => {
    console.log(actual, expected);
    expect(actual).to.deep.equal(expected);
  });

  const hotAction$ = testScheduler.createHotObservable(...action);
  const action$ = new ActionsObservable(hotAction$);
  const responseSubs = '^!';
  const response$ = testScheduler.createColdObservable(...response);
  const call = createCall(response$);

  const test$ = epic(action$, store, call);
  testScheduler.expectObservable(test$).toBe(...expected);
  testScheduler.flush();

  expect(call.calledOnce).to.be.true;
  expect(call.calledWithExactly(...callArgs)).to.be.true;

  testScheduler.expectSubscriptions(response$.subscriptions).toBe(responseSubs);
};

// Usage

it('calls the correct API', () => {
  const response = { id: 123, name: 'Bilbo' };

  expectEpic(fetchFooEpic, {
    expected: ['-a|', {
      a: { type: 'FETCH_FOO_FULFILLED', payload: response }
    }],
    action: ['(a|)', {
      a: { type: 'FETCH_FOO', payload: { id: 123 } }
    }],
    response: ['-a|', {
      a: response
    }],
    callArgs: [api.fetchFoo, 123]
  });
});

It feels gross to be testing the same behavior repeatedly, for each test. Wonder if there's a way to test each part individually, I'm just worried that if you do that it won't be exposing bugs that only happen as part of the various integration points since Epics are inherently complex.

jayphelps commented 7 years ago

@shuhei any additional thoughts? I'm admittedly not a "testing guru" so some of the things I do are naive.

gnoff commented 7 years ago

Just chiming in here to say that I am very interested in where this goes. I am starting a new project and had initially thought I would use redux-saga but then came across redux-observable. the main draw at this point for me is that with saga the tests can inject mock responses with no overhead and sagas don't have to take any DI arguments for mocks.

I guess the justification for why this test setup works is that if you effect code is tested, redux-saga is tested, and your saga itself is tested then there isn't too big of a surface area left untouched. Really just the interfaces between the saga and effect runner within the middleware and the implicit dependency between the real effect api and whatever you inject manually. The latter of course is true even if you provide mock objects to an Epic though since your real interface can change without your mock interface needing to update.

I like the injectable call solution, I think that probably comes closest to the ergonomics of redux-saga testing without actually extracting an effect runner.

jayphelps commented 7 years ago

@gnoff I too would like the same ease of testing as redux-saga. after a ton of tinkering I worry we're mostly trying to fit a square peg in a round hole though. I think it's OK to do this for API calls, but redux-saga also uses these Effects for things like debouncing/takeUntil, etc. If you do that with redux-observable, you no longer really using RxJS and lose all the ease it provides.

There's just some fundamental differences between the two approaches. We do actions in, actions out, they do actions in, effects out (which are POJOs, similar to actions but specific for effects).

I'm still exploring similar approaches, but we should prolly think further outside the existing redux-epic box/pattern.

gnoff commented 7 years ago

@jayphelps having not used rxjs yet and certainly having not set up a competent unit test scheme for observables I don't really have an intuition on what is customary. Regardless of epics and redux etc... is it common to expose side-effecting observables with a factory function that takes in dependencies whereby tests can supply mock observables? With very little to go off of this is the only apparent solution that doesn't feel dirty.

I get that for network type side effects you can always intercept at a higher level using nock or similar but that bleeds unnecessary concerns into the unit test and is something I think probably ought to be avoided where possibly. (like when testing for your own code)

Any advice?

gnoff commented 7 years ago

btw, i will say I have looked at TestSchedulers and such and this is what the majority of rxjs unit testing articles are about and they seem really cool and powerful, just not the right knob to twist when dealing with side effects. (unless there is something there i don't know)

jayphelps commented 7 years ago

@gnoff keep in mind that unfortunately most tutorials out there are referring to RxJS v4 not v5--the TestScheduler in v5 is very different and in many ways better, but we're all surfing the bleeding edge here so it's mostly undocumented. ๐Ÿ˜ข

As a general guideline, we recommend users come into redux-observable already having a solid Rx understanding because it's non-trivial to learn and redux-observable really just makes it easy to utilize that knowledge. So we are not ashamed to say that, if a person's async logic isn't really all that complex and they don't have Rx experience, they should prolly be using redux-thunk. In fact, very soon we're going to support running alongside redux-thunk and suggest that people new to Rx should use redux-thunk primary and only lean on redux-observable for more complex async logic (debouncing, throttling, cancelling, etc). Particularly if others on your team are in the same boat. Especially for new projects, there's a bit of a tendency for some to prematurely bring in some complex async dependency they have never used before; be careful you really do need such a thing. All that said, there's nothing wrong with using redux-observable for everything, if you and your team understand Rx this is natural.

We've given similar advice when people ask whether they should use redux-saga or redux-observable. If you're not familiar with Rx and don't currently see the need to learn it, you're probably better off using redux-saga at the moment since it's better documented and arguably easier to learn if you "get" generators. We don't want to scare people away though! Certainly this is very usable (obviously cause we use it) but this is new and we're all learning together.

Circling back to your question

is it common to expose side-effecting observables with a factory function that takes in dependencies whereby tests can supply mock observables?

I'm not entirely sure what you're asking, but my hunch is a bit of confusion between Epics and Observables/streams. Epics are functions which take a stream of actions and return a stream of actions. They are factories themselves and really "Epic" is just a fancy word we applied for our specific type of factory.

Factories are pretty common in Rx, the core library itself uses factories..like when you do Observable.of(1, 2, 3) or Observable.ajax('/user') those are factories. Using argument dependency injection is also fairly common both in core and in the wild. Schedulers are passed in as arguments to override the default scheduling behavior, which is why you can use TestScheduler too, same principle. This is more traditional than the Effect pattern redux-saga uses that was inspired by the Elm language. That Effects pattern is what we're discussing here and I haven't seen it used in Rx world.

gnoff commented 7 years ago

thanks @jayphelps

clarifying my question a bit more, let's say I have a non-observable async api interface that returns promises. (Or err first callback or whatever, this part isn't important)

given myApiClient.fetchUser(userId) // returns a promise

I imagine one can wrap this fairly easily as an observable. I'm not sure if this works but let's pretend this does: Observable.of(myApiClient.fetchUser(id))

if an epic of mine does something like

const fetchUser = myApiClient.fetchUser

export const myEpic = action$ =>
  action$
    .ofType(FETCH_USER)
    .mergeMap(action =>
      Observable.of(fetchUser(id))
        .map(user =>  ({ type: FETCH_USER_FULFILLED, user }))
        .takeUntil(action$.ofType(FETCH_USER_CANCELLED))

then if i wanted test this without actually fetching a real user from the real api

export const makeFetchUserEpic = fetcher => {
  return action$ =>
    action$
      .ofType(FETCH_USER)
      .mergeMap(action =>
        Observable.of(fetcher(id))
          .map(user =>  ({ type: FETCH_USER_FULFILLED, user }))
          .takeUntil(action$.ofType(FETCH_USER_CANCELLED))
}

// the real fetchUserEpic
export const myFetchUserEpic = makeFetchUserEpic(myApiClient.fetchUser)

// in some test file
import makeFetchUserEpic from '.....'
const myFetchUserEpic = makeFetchUserEpic(() => Promise.resolve({dummy: 'user'}))

If one doesn't do something like the above then I imagine the mocking needs to live in the myApiClient.fetchUser which is less accessible from within the Epic unit test (doing nasty stuff with globals or intercepting network calls which isn't my cup of tea)

gnoff commented 7 years ago

This is all standard DI stuff, just not sure if it is customary. I haven't seen an article or example discussing this (granted I haven't spent a ton of time looking) and I wasn't sure if it was because this is so routine that people just assume it or if there was some other idiomatic way of testing side-effecty streams that I don't have knowledge of.

I guess the one difference my scenario provides is that I would want to keep knowledge of the actual remote api hidden from the epic and therefore not use ajax directly in the epic to avoid leaking remote api shape to individual epics.

jayphelps commented 7 years ago

@gnoff Side note: consuming a promise as an Observable is done with Observable.from(promise). All operators that accept an Observable also accept a Promise, so mergeMap(() => promise) etc works too, but very often you need to apply Rx operators to it inside that mergeMap not outside of it so wrapping it in from(promise) is the way to go in that case.

As far as your factory of epic pattern, that's definitely acceptable. Some people mock via require a la Jest, others mock XMLHttpRequest, others do this or a similar form of this.

I'm personally using default params so I don't have to make a factory for the epic itself.

import * as api from './wherever/api';

export const myEpic = (action$, store, { fetchUser } = api) =>
  action$.ofType(FETCH_USER)
    .mergeMap(action =>
      Observable.from(fetchUser(id))
        .map(user =>  ({ type: FETCH_USER_FULFILLED, user }))
        .takeUntil(action$.ofType(FETCH_USER_CANCELLED)
    );

and testing (without TestScheduler):

myEpic(action$, mockStore, { fetchUserMock }).subscribe(...);

If you have control over your API utility functions, I highly recommend having them use Observables instead of Promises though since Promises are not truly cancellable. Observable.ajax and friends are.

gnoff commented 7 years ago

@jayphelps thanks, so as long as the redux-observable isn't updated to use a 3rd argument this solution is safe. (presumably this would be considered a breaking change)

I'll look into ajax for the api client, we do control it so having real cancellations is certainly nice and we already expose a Promise, and callback interface the internals switching to rxjs with a observable interface shouldn't be too challenging since consumers don't have to know about it.

thanks for the education, this has been interesting and helpful

jayphelps commented 7 years ago

@gnoff You're welcome! We have no immediate plans to add a third param, but it's possible. Until we reach 1.0.0 we treat minor versions as breaking, so lock to patch version if that's a concern.

https://github.com/ReactiveX/rxjs/issues/556 may or may not interest you if you want the best of both worlds in regards to Observable or Promise.

Feel free to join us on our gitter channel