Closed psamusev closed 6 years ago
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.
Ok, these are several examples. How error influence on Observer:
1) 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
2) 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.
3) 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
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?
@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.
@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.
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.
@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
@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.
@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.
@mpeyper so your https://github.com/ioof-holdings/redux-subspace/issues/76#issuecomment-362567260 is same as https://github.com/ioof-holdings/redux-subspace/issues/76#issuecomment-362551823 :)
I added https://github.com/ioof-holdings/redux-subspace/issues/76#issuecomment-362557436 as an alternative which can be used for removing workaround with deps.
@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.
This has been fixed in v2.3.1. Thanks to both of you for all of your help with this one.
@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).
I would like to add error handling for all of my epics for this I use
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