Odonno / ReduxSimple

Simple Stupid Redux Store using Reactive Extensions
http://www.nuget.org/packages/ReduxSimple/
MIT License
144 stars 20 forks source link

Exception during effect causing effect to be removed as action handler #85

Closed SonnyCampbellUnity closed 3 years ago

SonnyCampbellUnity commented 3 years ago

Hi, I'm not certain if this is expected behaviour, or if I should be doing something different to handle exceptions, but I'm having an issue where an exception getting thrown during an effect is causing the effect to get removed as an action handler and will no longer get triggered by further actions.

I was playing with the sample demo and updated the LoadPokemonById to the following:

public static Effect<RootState> LoadPokemonById = CreateEffect<RootState>(
    () => Store.ObserveAction<GetPokemonByIdAction>()
        .Select(action => _pokedexApiClient.GetPokemonById(action.Id))
        .Switch()
        .Select(response =>
        {
            throw new Exception('Test'); // ---- THIS EXCEPTION ADDED
            return new GetPokemonByIdFullfilledAction
            {
                Pokemon = new Pokemon
                {
                    Id = response.Id,
                    Name = response.Name.Capitalize(),
                    Image = response.Sprites.Image,
                    Types = response.Types
                        .OrderBy(t => t.Slot)
                        .Select(t => new PokemonType { Name = t.Type.Name })
                        .ToList()
                }
            };
        })
        .Catch<object, Exception>(e =>
        {
            return Observable.Return(
                new GetPokemonByIdFailedAction
                {
                    Exception = e
                }
            );
        }),
    true
);

Once this gets called the first time, it throws the Test exception and catches it in the Catch block. However if I then type another Id into the textbox of the sample app, the GetPokemonByIdAction is dispatched once more, but this effect never runs again.

Is this expected behaviour? And if so, what is the correct pattern for handling exceptions in an effect but allowing the effect to continue handling actions?

Odonno commented 3 years ago

Oh my! Looks like I miss a Retry somewhere. Will fix that and publish a new version.

Thank you very much for your feedback @SonnyCampbellUnity

SonnyCampbellUnity commented 3 years ago

@Odonno thanks for the quick reply! :)

Let me know when there's an updated version and I'll try it in my own app. I discovered the issue there first and wanted to replicate it in the demo project to make sure it wasn't something silly I was doing.

Odonno commented 3 years ago

The issue is now fixed. The v3.5.2 is now available.

@SonnyCampbellUnity Tell me if it works as expected. Have a nice day.

SonnyCampbellUnity commented 3 years ago

Hey @Odonno this doesn't seem to have fixed the issue for me :/

I've grabbed the latest version of the code and made sure the .Retry() fix you added is there, but if you run the sample app and set a breakpoint on the exception I added above, after the exception gets thrown once it never hits it again. Would you be able to take another look and let me know if I'm misunderstanding something?

Odonno commented 3 years ago

And all your tests are green?

Odonno commented 3 years ago

Oh well, another mistake. If you place a Catch in the root part of the Observable, it will become the new root Observable. And so it will complete the Effect once the Observable in the Catch is completed...

You'd have to put the Catch inside the Select which what I will do now. it should also match the API of the ngrx ecosystem (but with Rx.NET of course).

Odonno commented 3 years ago

It is solved. Can you check on the latest version of the source code @SonnyCampbellUnity ?

SonnyCampbellUnity commented 3 years ago

@Odonno Thanks a million, will try again!

FYI, I've actually replaced Rx.NET with UniRx to make ReduxSimple compatible with my Unity application (Rx.NET has compatibility issues with Unity so a team created UniRx achieve the same ngrx pattern in Unity). It has been essentially a drag-and-drop replacement - I just needed to update the usings in the relevant files and make one or two very minor adjustments to some code using generics.

At some point I'm going create a fork of this repo and update it with the changes I've made to make it compatible with the Unity Engine if you'd be interested in having a look?

Odonno commented 3 years ago

Oh ok. What's your use case for this lib in a Unity application?

And feel free to fork the repo if you want to. I could help you, yes, I'll try to be as useful as I can be.

SonnyCampbellUnity commented 3 years ago

I'm building a non-game mobile app with the Unity Engine for its graphics and rendering pipeline, and my background is mostly in web development so Redux is a pattern i'm relatively familiar with for app state management.

In the latest changes it works if the exception is thrown where I indicated in the code sample above, but if the error is thrown inside the api call the exception is totally swallowed. And if I add a Catch back onto that Select we get the same issue again with the effect not running at all the second time.

public static Effect<RootState> LoadPokemonById = CreateEffect<RootState>(
    () => Store.ObserveAction<GetPokemonByIdAction>()
        .Select(action =>
            _pokedexApiClient.GetPokemonById(action.Id) // <---- Exception thrown in here
                .Select(response =>
                {
                    return new GetPokemonByIdFullfilledAction
                    {
                        Pokemon = new Pokemon
                        {
                            Id = response.Id,
                            Name = response.Name.Capitalize(),
                            Image = response.Sprites.Image,
                            Types = response.Types
                                .OrderBy(t => t.Slot)
                                .Select(t => new PokemonType { Name = t.Type.Name })
                                .ToList()
                        }
                    };
                })
                .Catch<object, Exception>(e =>
                {
                    return Observable.Return(
                        new GetPokemonByIdFailedAction
                        {
                            Exception = e
                        }
                    );
                })
        )
        .Switch(),
    true
);
SonnyCampbellUnity commented 3 years ago

The only way I can seem to get it to handle the errors as I expect is to do something like

public static Effect<RootState> LoadPokemonById = CreateEffect<RootState>(
    () => Store.ObserveAction<GetPokemonByIdAction>()
        .Select(action =>
            try
            {
                return DoThingWithAction(action);
            }
            catch (Exception e)
            {
                return Observable.Return(
                        new GetPokemonByIdFailedAction
                        {
                            Exception = e
                        }
                    );
            }
        )
        .Switch(),
    true
);
Odonno commented 3 years ago

Yep, it looks like it. I had some trouble on an app with a similar pattern. I do not know how to fix this other than using try/catch...

SonnyCampbellUnity commented 3 years ago

Ok cool, that'll work for me anyway! I appreciate the help.

mrutter commented 3 years ago

BTW, thank you everybody (@SonnyCampbellUnity, @Odonno) for this post. It helped me to solve the same issue described here (v. 3.6.1 + moving the Catch inside the inner Select).