Closed kuhnroyal closed 4 years ago
I was also using a lot of return null;
in reducers but due to my reduceWithState
usage, I started to also use return state;
so that I can reduce multiple actions in a row without dispatching them.
For both sync and async reducers, returning a new state is optional. If you don't plan on changing the state, simply return null. This is the same as returning the state unchanged.
But there is a difference for async reducers, returning null
behaves correctly, but returning state
introduces the misbehaviors mentioned in
When your reducer returns Future
you must make sure you do not return a completed future. In other words, all execution paths of the reducer must pass through at least one await keyword.
I should probably remove reduceWithState
because it's very easy to make a mistake when using it (and if I don't remove it, I should fix it as you described it).
Can you create for me a runnable test, with minimum code, that demonstrates the problem you are having? (without using reduceWithState
)?
I'll try tomorrow.
import 'package:async_redux/async_redux.dart';
import "package:test/test.dart";
class AppState {
final String name;
AppState(this.name);
AppState copyWith(final String name) => AppState(name);
}
class Action1 extends ReduxAction<AppState> {
@override
Future<AppState> reduce() async {
await dispatchFuture(Action2());
// Action2 wraps the thrown argument error in a UserException thus this reduce never fails.
// This may be intented but Action1 never knows what happened.
return AppState('Action1');
}
}
class Action2 extends ReduxAction<AppState> {
@override
Future<AppState> reduce() async {
await Future.delayed(const Duration(milliseconds: 100), () {
throw ArgumentError();
});
return AppState('Action2');
}
@override
Object wrapError(dynamic error) {
return const UserException('error');
}
}
void main() {
test('Action throws', () async {
final store = StoreTester(initialState: AppState('FOO'));
store.dispatchFuture(Action1());
final info = await store.waitUntilErrorGetLast(error: ArgumentError);
print(info);
expect(info.error, isNotNull);
expect(info.errors, hasLength(1));
expect(info.dispatchCount, 2);
expect(info.state.name, 'FOO');
});
}
The error wrapping my also happen in the global WrapError
handler so that Action2
has no influence over wether the exception should be wrapped. Action2
may be an action that is used for its side effects from several other actions and its error should be displayed to the user. However if this action fails, the other (Action1) actions can not proceed.
Yes, UserException
s being swallowed (not throwing) is per design. I could create a special user exception class that is displayed to the user but not swallowed. This would solve the above problem but would create another: it would throw the error even when Action2
is not used inside of Action1
, which I guess is not what you want.
Please get version: [2.12.1] (directly from GitHub), and try this:
var action2 = Action2();
await dispatchFuture(action2);
if (!action2.hasFinished) throw UserExceptionError("Problems here.");
I don't think this is a good solution, but at the moment I can't think of a good one.
What do you think?
I will try this but I am also not sure that it is a good idea. The correct way would probably be to detect from the resulting state, if the action was successful, this however is not feasible for me in all cases.
My main problem is the handling in the WrapError
object which I use extensively.
I just had the idea to create some custom BaseAction that has a bool isSubAction
which indicates if the action was fired from within another action or from the top-level dispatch. I could then check on the action object inside WrapError
if the error should be rethrown or handled. This would require the action to be available inside WrapError
just like in ErrorObserver
.
Maybe this flag could also go into the ReduxAction
and the dispatch/dispatchFuture
that is available inside an action can set this to true by default. The flag can then be checked against in either ReduxAction.wrapError
or in WrapError
.
The default behavior stays the same so this is not breaking.
Does this sound sane?
Edit: Maybe the flag should be the other way around, like isRoot/TopLevelAction
or something.
I can add the action to the global WrapError
, yes, that could be useful for other use cases as well.
However, with the ErrorObserver
alone you can already do what you described above. Define your own ErrorObserver
that will check if your action is of type MyBaseAction
, and if so, check your isSubAction
flag. If you decide you want the UserException
to throw, simply return true
from the ErrorObserver
.
Yes I know about the ErrorObserver
and that I can force it to throw, however I need to prevent the exception from the inner action to be wrapped as UserException
at all, otherwise I have 2 dialogs.
I think it would help me a lot if the action is available in WrapError
.
Something like this should work I guess:
abstract class CustomReduxAction<St> extends ReduxAction<St> {
bool _isTopLevel = true;
bool get isTopLevel => _isTopLevel;
@override
DispatchFuture<St> get dispatchFuture {
return (ReduxAction<St> action, {bool notify}) {
if (action is CustomReduxAction<St>) {
action._isTopLevel = false;
}
return super.dispatchFuture(action, notify: notify);
};
}
@override
Dispatch<St> get dispatch {
return (ReduxAction<St> action, {bool notify}) {
if (action is CustomReduxAction<St>) {
action._isTopLevel = false;
}
return super.dispatch(action, notify: notify);
};
}
}
Do you think this is something that makes sense to be added to the library?
No, I don't really think it makes sense to add it to the library. I think that's too confusing to use, and nothing prevents the caller to call store.dispatch
directly, therefore bypassing the action's own dispatch
method. But I think this may be a good solution for you, since you developed it yourself and it won't be confusing for you.
Please see version [2.12.2] (in GitHub, not pub.dev yet) for WrapError
with action, and deprecated reduceWithState
.
I'm going to close this issue, but feel free to come back here with other ideas, in case you find them.
Sounds good, will try from master later.
Don't forget the original problem with reduceWithState
:)
ReduxAction.reduceWithState
is not working as the_store
instance hasn't been set yet. It is also no longer mentioned in the docs so I am not sure if this is still intended to work.I am currently working around the problem with a simple extension:
My main use case for using this is that I have an action
Action1
which dispatches other actions in their reducer like this:await dispatchFuture(Action2())
IfAction2
throws an exception which is wrapped in aUserException
thenAction1
does not get canceled.So instead I use
await Action2().reduceWith(store, state)
so that it throws but now I have to handle the wrapping of the exception also inAction1
.I am not sure what is the best way to tackle this.