mrpmorris / Fluxor

Fluxor is a zero boilerplate Flux/Redux library for Microsoft .NET and Blazor.
MIT License
1.25k stars 144 forks source link

Dispatch -> Reduce -> Effect sequence #226

Closed uhfath closed 2 years ago

uhfath commented 2 years ago

Is this sequence always like this? When dispatching an action is Reducer called right away and then effects in async manner? I'm asking because I've stumbled upon an issue because made an assumption that this sequence is always the same. https://github.com/uhfath/TestFluxorLifecycle

After running, open console and navigate to "Counter" page. This is what is output during "normal" flow:

DISPATCH_START_QUERY
START_QUERY_REDUCE
START_QUERY_EFFECT
DISPATCHED_START_QUERY
START_QUERY_EFFECT_RESULT
START_QUERY_EFFECT_RESULT_AWAITED
DISPATCH_START_QUERY_RESULT: 10
DISPATCH_FINISH_QUERY
FINISH_QUERY_REDUCE
DISPATCHED_FINISH_QUERY

The problem happens when navigating to the page directly through browser refresh. When on a "counter" page just refresh and this is what comes out:

DISPATCH_START_QUERY
DISPATCHED_START_QUERY
DISPATCH_START_QUERY_RESULT: -1
DISPATCH_FINISH_QUERY
DISPATCHED_FINISH_QUERY
START_QUERY_REDUCE <-- this should have been earlier
START_QUERY_EFFECT
FINISH_QUERY_REDUCE
START_QUERY_EFFECT_RESULT
START_QUERY_EFFECT_RESULT_AWAITED

Are there any nuances regarding events sequence?

uhfath commented 2 years ago

I'll explain what I'm after. There is a 3rd party component (a data table) that requests some data inside it's own async callback (table rows). That's the only way to supply them. I wanted to use Fluxor to keep these rows as part of the state (to save them between app navigations).

The idea is this:

  1. create a state with a Task<Items> Items property which is initially Task.FromResult(DefaultIEmptytems)
  2. inside component's callback dispatch an action to start query process and start awaiting state's Items
  3. an action contains a TaskCompletionSource<Items> with which state's Items property is replaced inside a reducer
  4. an attached effect is executed which queries the data and applies it to the state through TaskCompletionSource.SetResult
  5. the Items task is completed and data is passed to component inside it's callback

The important part here is the sequence in which reducers are executed and it seems to be exactly as I expected it would be. But it changed when page is reloaded and not navigated directly.

uhfath commented 2 years ago

Could it be something related to the way how store is initialized? https://github.com/mrpmorris/Fluxor/blob/c2b926f0238fa51d0aa7af8d5b0b0ec18ad3dc32/Source/Fluxor.Blazor.Web/StoreInitializer.cs#L81 It's just that our 3rd party component also calls the data query callback inside it's own OnAfterRenderAsync (hence why I've made the same in the repro). When using await Task.Yield() inside InvokeCallbackAsync as a first statement, then the sequence works as expected.

mrpmorris commented 2 years ago

I don't know how your rendering can affect the order of action/effect handling

szalapski commented 2 years ago

Isn't it true that the effect is executed in a Task.Run and therefore might be in a non-deterministic race against other actions in OnAfterRender? Which would mean, in order to reliably know when the effect is complete, one would have to call a different action that has a reducer that will set a value (presumably a boolean for the simplest case) on the state?

mrpmorris commented 2 years ago

The effects are executed all at once and then awaited. If you want to update state as a consequence of the effect completing then the effect must dispatch an action.

szalapski commented 2 years ago

"The effects are executed all at once and then awaited.", yes, which is to say, they might complete before, during, or after other component code runs. Right?

uhfath commented 2 years ago

I believe that the store initialization is the problem. It happens inside OnAfterRenderAsync and is not complete when user action is dispatched from another component's OnAfterRenderAsync

mrpmorris commented 2 years ago

Any dispatched actions are queued up until the store is initialized, and then they are required.

Is this causing a problem?

uhfath commented 2 years ago

Looks like it is. The actions are actually not reduced until initialization is complete. As a consequence we can't depend on the order of execution. Is it possible to move initialization somewhere else out of components lifecycle?

mrpmorris commented 2 years ago

Store Initialization must be done in OnAfterRenderAsync, because it executes any javascript injected by middleware.

When you open / the store will be initialized before you navigate to /counter, so dispatching StartQueryCounterAction will reduce before your call to Dispatch has finished.

When open navigate directly to /counter this is what happens

  1. StoreInitializer.OnAfterRenderAsync does an await that does not complete immediately
  2. Counter.OnAfterRenderAsync dispatches (which is a synchronous action)
  3. Counter.OnAfterRenderAsync does an await that does not complete immediately
  4. StoreInitialize.OnAfterRenderAsync's await completes, and then dequeues the dispatched action in #2 so it gets reduced

The two processes should result in exactly the same result to your state.

You seem to be using mutable state (having a task in state that you complete is mutating it). I have altered your code below so no mutations are needed.

/* ========================== */

using Fluxor;

namespace TestFluxor.States
{
    public record CounterState
    {
        public int Counter { get; init; } 
        public bool IsLoading { get; init; }

        private class Feature : Feature<CounterState>
        {
            public override string GetName() =>
                nameof(CounterState);

            protected override CounterState GetInitialState() =>
                new();
        }
    }
}

/* ========================== */

@inherits FluxorComponent
@page "/counter"

<h1>Counter</h1>
<p>Current count: @(CounterState.Value.IsLoading ? "Loading..." : CounterState.Value.Counter.ToString())</p>

/* ========================== */

using Fluxor;
using Fluxor.Blazor.Web.Components;
using Microsoft.AspNetCore.Components;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using TestFluxor.Actions;
using TestFluxor.States;

namespace TestFluxor.Pages
{
    public partial class Counter: FluxorComponent
    {
        [Inject]
        private IState<CounterState> CounterState { get; set; }

        [Inject]
        private IDispatcher Dispatcher { get; set; }

        private void InvokeCallback()
        {
            Console.WriteLine("StartQueryCounterAction: Dispatch start");
            Dispatcher.Dispatch(new StartQueryCounterAction());
            Console.WriteLine("StartQueryCounterAction: Dispatch complete");
        }

        protected override void OnAfterRender(bool firstRender)
        {
            base.OnAfterRender(firstRender);
            if (firstRender)
                InvokeCallback();
        }
    }
}

/* ========================== */

using Fluxor;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using TestFluxor.States;

namespace TestFluxor.Actions
{
    public record StartQueryCounterAction
    {
        protected readonly TaskCompletionSource<int> CounterValueSource = new(TaskCreationOptions.RunContinuationsAsynchronously);

        [ReducerMethod]
        public static CounterState Reduce(CounterState counterState, StartQueryCounterAction refreshCurrentCounter)
        {
            Console.WriteLine("StartQueryCounterAction: Reduce");
            return counterState with
            {
                Counter = 0,
                IsLoading = true
            };
        }

        private class Effect : Effect<StartQueryCounterAction>
        {
            public override async Task HandleAsync(StartQueryCounterAction action, IDispatcher dispatcher)
            {
                Console.WriteLine("StartQueryCounterAction: Effect start");
                await Task.Delay(1000);

                Console.WriteLine("StartQueryCounterAction: Effect complete");
                dispatcher.Dispatch(new FinishQueryCounterAction(10));
            }
        }
    }
}

/* ========================== */

using Fluxor;
using System;
using TestFluxor.States;

namespace TestFluxor.Actions
{
    public record FinishQueryCounterAction
    {
        public int Counter { get; init; }

        public FinishQueryCounterAction(int counter)
        {
            Counter = counter;
        }

        [ReducerMethod]
        public static CounterState Reduce(CounterState counterState, FinishQueryCounterAction setCounterAction)
        {
            Console.WriteLine("FinishQueryCounterAction: Reduce");
            return counterState with
            {
                Counter = setCounterAction.Counter,
                IsLoading = false
            };
        }
    }
}
uhfath commented 2 years ago

Actually the need to have a Task in my state is described in my comment. There are cases when we need to update the state and await for it at the same time inside a callback, hence why the sample is organized that way. The callback itself is async and is called from a 3rd party component. I just extracted the code from it here to simplify things. If you could suggest another workaround I would be very grateful. Perhaps there are more cases when using some 3rd party components that request their data only in a callback.

mrpmorris commented 2 years ago

I use Telerik. They allow me to push the result when I an ready just by setting the data + page number + page count. Can you do that?

uhfath commented 2 years ago

Nope. The callback contains the page index, page size and sorting information as input parameters and as a result awaits for data. When sorting or page index changes through user interaction the callback is invoked with new parameters.

mrpmorris commented 2 years ago

Which grid is it, and do they have a trial?

uhfath commented 2 years ago

https://mudblazor.com/wasm/components/table#api

It's an open source component pack. There are 2 ways to supply data. It's either directly supplying entire dataset (all rows at once), or through a callback to query API for data.

Actually the same approach is used in another commercial component pack which we use in other projects - DevExtreme so it's not uncommon I believe.

mrpmorris commented 2 years ago

I can't work out mudblazor table data paging. Can you create a server-side blazor app for me that simply uses the WeatherForecastService to page data?

uhfath commented 2 years ago

Sure thing! Here: https://github.com/uhfath/TestMudBlazorFluxor Tried to make it as simple as possible but with all the required stuff included.

uhfath commented 2 years ago

@mrpmorris, actually after thinking about it a bit more I've came to another solution: https://github.com/uhfath/TestMudBlazorFluxor/tree/another_completion_source

After your comment about mutability of the state because of the Task property I've moved TaskCompletionSource out of it. And it seems to work fine even without Task.Yield() workaround since we are not dependent on the sequence of events now.

Could you please share your thoughts about this?

mrpmorris commented 2 years ago

Which are the relevant parts of the code?

uhfath commented 2 years ago

Probably here where the TaskCompletionSource is created outside of action dispatch. That's where the full ownership of it is taken. And here where the Task is actually resolved right after data is ready.

The main question here is - where should the Task be resolved with the actual data? The choices that I see are:

  1. UpdateWeatherForecastAction - could it be considered as a side effect?
  2. UpdateWeatherForecastAction.Reduce - same question.
  3. UpdateWeatherForecastAction.Effect - takes too long to complete.
  4. QueryWeatherForecastAction.Effect - right after data is ready but before an update action is reduced.
mrpmorris commented 2 years ago

My preference would be to write to the library authors and ask them to provide a way of pushing the current page's items + page number + total number of pages - as Telerik do.

If not, then the approach you have is exactly what I would have suggested. I wouldn't have the task or completion source in state because it is mutable, and I therefore wouldn't complete it in a reducer because it isn't state (reducers should ideally have no side effects).

Because it has a side effect, put it in an effect, just as you have.

PS: Did you know there is an [EffectMethod] technique? There is now also a [FeatureState] alternative to writing Feature<T> descendents.

uhfath commented 2 years ago

I get your point, thanks. Unfortunately that's not the only library/component which we use and which queries for data, so we'll use this approach.

PS: thanks for the tip, will consider that.