Subspaced epic should re-throw an error

Question

Subspaced epic should re-throw an error

psamusev opened this issue 6 years ago · 13 comments

I would like to add error handling for all of my epics for this I use

epic(action, store, deps).pipe(
     catchError((error, stream) => {
         / custom error handling
         return stream;   // return stream to make epic alive after any error
})

when I use it outside of subspace all of my errors appear in this ** catchError** callback.
But when epic is executed from subspace epic error is swallowing and catchError is not executing. By this epic is dead but I need keep it alive.

epic which is returning by subspaced should re-throw an error if it happened in original epic

Answer 1 · 2018-01-31T21:54:22.000Z

Given how the subspaced wrapper is implemented, it's not surprising that an error raised in the original epic cannot be caught from the outside.

I'm not very experienced with using rx myself so can you please elaborate on the use case here? I'm trying to understand what you might be doing in your custom error handling and what "return stream to make epic alive after any error" means.

My concern is that if the subspaced wrapper emits the error and you handle it, then the epic running inside the wrapper is too isolated to get any benefit.

If you have any ideas on how subspaced can be modified to better meet your requirements, I'm more than happy to discuss that here or in a PR.

Answer 2 · 2018-02-01T07:46:07.000Z

Ok, these are several examples. How error influence on Observer:

If inside it you coutch an error Observer would terminate - it means that sequence of this observable is no more handling.

For instance we have an epic

const epic = (action, store) => 
action.ofType(SOME_TYPE)
     .mergeMap((action) => {
              if (someFlag) {
                        return Observable.of(value)
              }
              throw new Error('Wrong flag value');
     })

if we catch an error whole epic will be terminated. It means that during one more event dispatch of this event - SOME_TYPE - nothing will be happened

If inside it you coutch an error and even handled it with catch Observer would terminate as well.

For instance we have an epic

const epic = (action, store) => 
action.ofType(SOME_TYPE)
     .mergeMap((action) => {
              if (someFlag) {
                        return Observable.of(value)
              }
              throw new Error('Wrong flag value');
     })
     .catch((error) => {  // do some error handling })

if we catch an error whole epic will be terminated as well even with handling.

If inside it you coutch an error and handled it with catch and one important thing return the same stream epic will continue leave.

For instance we have an epic

const epic = (action, store) => 
action.ofType(SOME_TYPE)
     .mergeMap((action) => {
              if (someFlag) {
                        return Observable.of(value)
              }
              throw new Error('Wrong flag value');
     })
     .catch((error, stream) => {
             // do some error handling
            return stream;   // it is matter to keep epic alive
     })

if we catch an error epic will continuer proceed action dispatch because inside catch callback we return the same stream.

In my example I have the following

epic(action, store, {...allDependencies, ...dependencies})
      .catch((error, stream) => {
                    return stream;
       })

But if epic is your subspaced wrapper I will never reach the handling because you swallow an error and my original epic will be terminated. So that's why I need re-throw

Answer 3 · 2018-02-01T12:28:14.000Z

Hello

I was thought about a bit different implementation of subspaced epic decorator from the time when I found the #65 and also in context of #71 and again on this issue.

@mpeyper please look at the bellow proposal how the implementation of subspaced can look like. This implementation is passing all tests, but maybe there are some edge cases which I do not know.

import { namespacedAction } from "redux-subspace";
import { map } from 'rxjs/operator/map'
import { filter } from 'rxjs/operator/filter'

const hasNamespace = (action, namespace) => action && action.type && action.type.indexOf(`${namespace}/`) === 0;
const isGlobal = (action) => !action.type || action.globalAction === true;

const subspaced = (mapState, namespace) => {

    if (!namespace && typeof mapState === 'string') {
        namespace = mapState;
        mapState = (s) => s[namespace];
    }

    return epic => (action$, store, dependencies) => {
        let filteredAction$;
        if (namespace) {
            filteredAction$ = action$
                ::filter((a) => hasNamespace(a, namespace) || isGlobal(a))
                ::map((a) => isGlobal(a) ? a : {...a, type: a.type.substring(namespace.length + 1)});
        } else {
            filteredAction$ = action$;
        }

        return epic(
            filteredAction$, {getState: () => mapState(store.getState())}, dependencies)
            ::map(a => isGlobal(a) ? a : namespacedAction(namespace)(a))
    }
};

export default subspaced

This implementation simply wraps the epic instead of replacing the streams. As you can see I do not need the reference to parentStore, so I do not need the dependencies workaround.

@psamusev Do you think such solution will solve the problem with errors?

Answer 4 · 2018-02-01T12:59:41.000Z

@psamusev Do you think such solution will solve the problem with errors?

@majo44 if we just wrap an epic and it will be the same stream I think yes.

Answer 5 · 2018-02-01T13:09:07.000Z

@majo44 This would work so long as the only things epic ever wanted to dispatch were standard actions (i.e. plain objects with a type field). If something else was returned, e.g. a thunk, then the subspace namespacing would not get applied to whatever it then dispatched. To be fair, we don't have any tests that don't expect a stream of standard actions.

The biggest advantage of dispatching into a subspaced store is that it gets access to the whole middleware chain for that subspace level.

I've been playing around with this:

import { Observable } from 'rxjs/Observable'
import { map } from 'rxjs/operator/map'
import { filter } from 'rxjs/operator/filter'
import { subspace } from 'redux-subspace'
import { SUBSPACE_STORE_KEY } from './subspaceStoreKey'

const identity = (x) => x

const subspaced = (mapState, namespace) => {
    const subspaceDecorator = subspace(mapState, namespace)
    return epic => (action$, store, { [SUBSPACE_STORE_KEY]: parentStore, ...dependencies } = {}) => {
        if (parentStore === undefined) {
            throw new Error('Subspace epic couldn\'t find the store. Make sure you\'ve used createEpicMiddleware from redux-subspace-observable')
        }
        const subspacedStore = subspaceDecorator(parentStore)

        Object.defineProperty(dependencies, SUBSPACE_STORE_KEY, {
            enumerable: false,
            configurable: false,
            writable: false,
            value: subspacedStore
        })

        const filteredAction$ = action$
            ::map((action) => subspacedStore.processAction(action, identity))
            ::filter(identity)

        return Observable.create((observer) => {
            epic(filteredAction$, subspacedStore, dependencies)
                .subscribe(subspacedStore.dispatch, (error) => observer.error(error))
        })
    }
}

export default subspaced

It does allow a catch at the root level to handle and restart the epic, but I'm getting weirdness when an epic outside the subspace throws and then an action is dispatched to be handled inside the subspaced epic. The subspaced epic appears to be playing the stream multiple times (once for each error before an action is handled inside the subspace).

Any thoughts either of you may have on why that might be the case would be much appreciated.

Answer 6 · 2018-02-01T13:18:43.000Z

For reference, this is the test I'm using to try and work it out (the test passes (there are no assertions 😛), but check out how many times `'

it('should allow error handling in root epic', () => {

        const throwingEpic = (action$) => 
            action$.ofType(TEST_ACTION_TRIGGER)
                 ::mergeMap((action) => {
                    if (!action.throw) {
                      return of({ type: TEST_ACTION, value: action.throw })
                    }
                    console.log(`throwing ${action.message}`)
                    throw new Error(`expected ${action.message}`);
                 })

        const errorHandler = (epic) => (action$, store, deps) => {
            return epic(action$, store, deps)
                ::_catch((error, stream) => {
                    console.log(error.message)
                    return stream
                })
        }

        const rootStore = createStore(rootReducer, applyMiddleware(createEpicMiddleware(
            errorHandler(
                combineEpics(
                    throwingEpic,
                    subspaced((state) => state.parent2, 'parentNamespace')(throwingEpic)
                )
            )
        )))

        const tmpDispatch = rootStore.dispatch

        rootStore.dispatch = (action) => console.log("dispatched", action) || tmpDispatch(action)

        const parentStore = subspace((state) => state.parent2, 'parentNamespace')(rootStore)

        console.log("root dispatch 1")
        rootStore.dispatch({ type: TEST_ACTION_TRIGGER, throw: true, message: 'root 1' })
        console.log("root dispatch 1 done")
        console.log()

        console.log("parent dispatch 1")
        parentStore.dispatch({ type: TEST_ACTION_TRIGGER, throw: true, message: 'parent 1' })
        console.log("parent dispatch 1 done")
        console.log()

        console.log("root dispatch 2")
        rootStore.dispatch({ type: TEST_ACTION_TRIGGER, throw: true, message: 'root 2' })
        console.log("root dispatch 2 done")
        console.log()

        console.log("parent dispatch 2")
        parentStore.dispatch({ type: TEST_ACTION_TRIGGER, throw: true, message: 'parent 2' })
        console.log("parent dispatch 2 done")
        console.log()
    })

The output is:

root dispatch 1
dispatched { type: 'TEST_ACTION_TRIGGER', throw: true, message: 'root 1' }
throwing root 1
expected root 1
root dispatch 1 done

root dispatch 2
dispatched { type: 'TEST_ACTION_TRIGGER', throw: true, message: 'root 2' }
throwing root 2
expected root 2
root dispatch 2 done

parent dispatch 1
dispatched { type: 'parentNamespace/TEST_ACTION_TRIGGER',
  throw: true,
  message: 'parent 1' }
throwing parent 1
throwing parent 1
throwing parent 1
expected parent 1
parent dispatch 1 done

parent dispatch 2
dispatched { type: 'parentNamespace/TEST_ACTION_TRIGGER',
  throw: true,
  message: 'parent 2' }
throwing parent 2
expected parent 2
parent dispatch 2 done

You can see 'throwing parent 1' gets logged 3 times when I only expect it once.

Answer 7 · 2018-02-02T10:43:00.000Z

@mpeyper
Ok so for solving errors problem in your way probably such solution will work:

import { map } from 'rxjs/operator/map'
import { ignoreElements } from 'rxjs/operator/ignoreElements'
import { filter } from 'rxjs/operator/filter'
import { subspace } from 'redux-subspace'
import { SUBSPACE_STORE_KEY } from './subspaceStoreKey'

const identity = (x) => x

const subspaced = (mapState, namespace) => {
    const subspaceDecorator = subspace(mapState, namespace);
    return epic => (action$, store, { [SUBSPACE_STORE_KEY]: parentStore, ...dependencies } = {}) => {
        if (parentStore === undefined) {
            throw new Error('Subspace epic couldn\'t find the store. Make sure you\'ve used createEpicMiddleware from redux-subspace-observable')
        }
        const subspacedStore = subspaceDecorator(parentStore);

        Object.defineProperty(dependencies, SUBSPACE_STORE_KEY, {
            enumerable: false,
            configurable: false,
            writable: false,
            value: subspacedStore
        });

        const filteredAction$ = action$
            ::map((action) => subspacedStore.processAction(action, identity))
            ::filter(identity);

        return epic(filteredAction$, subspacedStore, dependencies)
                ::map(subspacedStore.dispatch)
                ::ignoreElements();
    }
};

export default subspaced

I tied to run your test and the output is:

root dispatch 1
dispatched { type: 'TEST_ACTION_TRIGGER', throw: true, message: 'root 1' }
throwing root 1
expected root 1
root dispatch 1 done

parent dispatch 1
dispatched { type: 'parentNamespace/TEST_ACTION_TRIGGER',
  throw: true,
  message: 'parent 1' }
throwing parent 1
expected parent 1
parent dispatch 1 done

root dispatch 2
dispatched { type: 'TEST_ACTION_TRIGGER', throw: true, message: 'root 2' }
throwing root 2
expected root 2
root dispatch 2 done

parent dispatch 2
dispatched { type: 'parentNamespace/TEST_ACTION_TRIGGER',
  throw: true,
  message: 'parent 2' }
throwing parent 2
expected parent 2
parent dispatch 2 done

Answer 8 · 2018-02-02T11:06:24.000Z

@mpeyper
I see the problem with non FSA actions, and my proposal.
As I understand epic to be subspaced has to consume FSA action, only the result of an epic can be non FSA object/function.

So such solution should work:

import { namespacedAction } from "redux-subspace";
import { map } from 'rxjs/operator/map'
import { filter } from 'rxjs/operator/filter'
import { SUBSPACE_STORE_KEY } from './subspaceStoreKey'
import { subspace } from 'redux-subspace'

const hasNamespace = (action, namespace) => action && action.type && action.type.indexOf(`${namespace}/`) === 0;
const isGlobal = (action) => !action.type || action.globalAction === true;
const isFSA = (action) => typeof action === 'object' && action.type;

const identity = (x) => x;

const subspaced = (mapState, namespace) => {

    const subspaceDecorator = subspace(mapState, namespace);
    if (!namespace && typeof mapState === 'string') {
        namespace = mapState;
        mapState = (s) => s[namespace];
    }

    return epic => (action$, store, { [SUBSPACE_STORE_KEY]: parentStore, ...dependencies } = {}) => {
        let subspacedStore;
        let filteredAction$;
        if (namespace) {
            filteredAction$ = action$
                ::filter((a) => hasNamespace(a, namespace) || isGlobal(a))
                ::map((a) => isGlobal(a) ? a : {...a, type: a.type.substring(namespace.length + 1)});
        } else {
            filteredAction$ = action$;
        }

        return epic(
            filteredAction$, {getState: () => mapState(store.getState())}, dependencies)
            ::map(a => {
                if (isFSA(a)) {
                    return isGlobal(a) ? a : namespacedAction(namespace)(a);
                } else {
                    if (parentStore === undefined) {
                        throw new Error('Subspace epic couldn\'t find the store. Make sure you\'ve used createEpicMiddleware from redux-subspace-observable')
                    }
                    if (!subspacedStore) subspacedStore = subspaceDecorator(parentStore);
                    subspacedStore.dispatch(a);
                    return false;
                }
            })
            ::filter(identity);
    }
};

export default subspaced

The difference is that in case when only FSA actions are used, we do not have to use dependencies workaround.

Answer 9 · 2018-02-02T11:55:55.000Z

@majo44, I believe you are right in that the epic will only ever receive plain actions. I think your proposal would for the standard case and we may have to fall back to something like this if a cleaner solution can't be found. My biggest concern is that this is effectively duplicating the subspace logic that the core library should be able to do for us.

Seeing this proposal gave me an idea, which is looking promising:

import { map } from 'rxjs/operator/map'
import { filter } from 'rxjs/operator/filter'
import { mergeMap } from 'rxjs/operator/mergeMap'
import { empty } from 'rxjs/observable/empty'
import { subspace } from 'redux-subspace'
import { SUBSPACE_STORE_KEY } from './subspaceStoreKey'

const identity = x => x

const subspaced = (mapState, namespace) => {
  const subspaceDecorator = subspace(mapState, namespace)
  return epic => (action$, store, { [SUBSPACE_STORE_KEY]: parentStore, ...dependencies } = {}) => {
    if (parentStore === undefined) {
      throw new Error(
        "Subspace epic couldn't find the store. Make sure you've used createEpicMiddleware from redux-subspace-observable"
      )
    }
    const subspacedStore = subspaceDecorator(parentStore)

    Object.defineProperty(dependencies, SUBSPACE_STORE_KEY, {
      enumerable: false,
      configurable: false,
      writable: false,
      value: subspacedStore
    })

    const filteredActions$ = action$
        ::map(action => subspacedStore.processAction(action, identity))
        ::filter(identity)

    return epic(filteredActions$, subspacedStore, dependencies)
        ::mergeMap(action => {
            subspacedStore.dispatch(action)
            return empty()
        })
  }
}

export default subspaced

Effectively, this is replacing the subscribe with mergeMap and returns the original epic chain, so any errors that occur in the original epic or the resulting dispatch bubble up the the root and there isn't a new stream to worry about that I think was causing the weirdness in my previous attempts.

The above appears to be working, but I'm still trying to write a decent test that actually proves it. You should be able to copy it locally to try it out and see if it's meeting your use case. Please let me know if there are any issues.

Answer 10 · 2018-02-02T12:04:40.000Z

@mpeyper so your #76 (comment) is same as #76 (comment) :)

I added #76 (comment) as an alternative which can be used for removing workaround with deps.

Answer 11 · 2018-02-02T12:13:35.000Z

@majo Oh damn, I didn't even see that comment (notification jumped straight to your second one) and I like yours better! It better describes the intent of what is going on.

How would you feel about submitting a PR? I feel like you should totally get the credit for this one. I'm not sure how exhaustive tests around the error cases would need to be though.

Answer 12 · 2018-02-04T00:42:46.000Z

This has been fixed in v2.3.1. Thanks to both of you for all of your help with this one.

Answer 13 · 2018-02-04T11:43:11.000Z

@psamusev I have recently adopted the all-contributors spec for this repo. I have added @majo44 already as github already recognises his contribution in the form of a commit.

I would like to include you in the contributors list under the "Bug reports" category.

If you do not want to be included, please let me know and I will not do anything.

If you do want to be included, you may submit a PR, following the guide on how to add yourself to the list. Alternatively, I can add you to the list (which I will do if you do not respond at all in a few days).