adospace / reactorui-maui

MauiReactor is a MVU UI framework built on top of .NET MAUI
MIT License
588 stars 49 forks source link

SetState in OnMount is broken with 1.0.146 #167

Closed Mithgroth closed 11 months ago

Mithgroth commented 11 months ago

I believe this comment is a valid concern.

await connection.StartAsync();

if (connection.State == HubConnectionState.Connected) { HttpClientHandler clientHandler = new() { ServerCertificateCustomValidationCallback = (sender, cert, chain, sslPolicyErrors) => true }; HttpClient client = new(clientHandler); client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", State.Token);

var jsonString = JsonConvert.SerializeObject(
    new
    {
        Operations = Operations.All,
        State.Id,
        State.Password
    });
var content = new StringContent(jsonString, Encoding.UTF8, "application/json");

var response = await client.PostAsync($"{AppConstants.BASE_URL}/api/stuff", content);
if (response.IsSuccessStatusCode)
{
    await Microsoft.Maui.Controls.Shell.Current.GoToAsync<ReportPageProps>(
        "report",
        props =>
        {
            props.Token = State.Token;
            props.Connection = connection;
        }
    );
}
else
{
    // TODO
}

}

* Navigates with shell to ReportPage, passes the SignalR connection, starts listening - the request may take a while so it's async.
```csharp
protected override void OnMounted()
{
    Task.Run(async () =>
    {
        Props.Connection.On<string>("FetchCompleted", (message) =>
        {
            Console.WriteLine($"Received message: {message}");
            var response = JsonConvert.DeserializeObject<FetchResponse>(message);
            if (response is null)
            {

            }

            switch (response.Operations)
            {
                case Operations.Something:
                {
                    SetState(_ => _.Responses?.Add(new(Operations.Something, response)));
                    if (response.Status == OperationStatus.Success && response.Result is JObject jObject)
                    {
                        SetState(_ => _.SomethingResult = jObject.ToObject<Something>());
                    }

                    break;
                }

                case Operations.AnotherThing:
                {
                    SetState(_ => _.Responses?.Add(new(Operations.AnotherThing, response)));
                    if (response.Status == OperationStatus.Success && response.Result is JObject jObject)
                    {
                        SetState(_ => _.AnotherThingResult = jObject.ToObject<AnotherThing>());
                    }

                    break;
                }

               ...
            }
        });
    });

    base.OnMounted();
}

1.0.145 I have these SetStates triggering, but on 1.0.146 they don't. Although my setup is irrelevant I added it hoping that it might help to visualize the issue better. I might be able to provide a sample if required.

adospace commented 11 months ago

yes, I'm trying to understand why the latest PR is creating these problems, so thanks for reporting this. Of course, if you could provide a sample reproducing that would be a huge help.

Anyway, based on the above code I can see that your approach is not recommended (I mean your code should work as well as it was in 1.0.145).

For production scenarios, I can suggest moving the code related to API connection + Signalr to a service to be injected into the component. As a general rule components should only contain code that renders UI and responds to events by the user: avoid any other kind of code, especially long-running background threads.

something like:

public class MyService : IMyService
{
SignalRManager.Connection _connection;

public event EventHandler<EventArgsWithMessage> NewMessage;

public async Task<bool> Connect()
{
    _connection = await SignalRManager.CreateConnection(State.Token);

    await _connection .StartAsync();

    if (_connection .State == HubConnectionState.Connected)
    {
        HttpClientHandler clientHandler = new() { ServerCertificateCustomValidationCallback = (sender, cert, chain, sslPolicyErrors) => true };
        HttpClient client = new(clientHandler);
        client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", State.Token);

        var jsonString = JsonConvert.SerializeObject(
            new
            {
                Operations = Operations.All,
                State.Id,
                State.Password
            });
        var content = new StringContent(jsonString, Encoding.UTF8, "application/json");

        var response = await client.PostAsync($"{AppConstants.BASE_URL}/api/stuff", content);
        if (response.IsSuccessStatusCode)
        {
            return true;
        }
        else
        {
            // TODO
        }
    }
    }
}

public void StartListening()
{
//if not connected throw invalid op
//if is already listening exit

        _connection .On<string>("FetchCompleted", (message) =>
        {
            Console.WriteLine($"Received message: {message}");
            var response = JsonConvert.DeserializeObject<FetchResponse>(message);
            if (response is null)
            {
                return;
            }

          NewMessage?.Invoke(this, new EventArgsWithMessage(message);
        });
}
class MainPage : Component
{
    async void OnConnectButtonClicked()
    {
      var myService = Services.GetService<IMyService>();
      var connected = await myService.Connect();

      if (connected)
      {
              await Microsoft.Maui.Controls.Shell.Current.GoToAsync(
                "report"
            );
      }
    }
}

class ReportPage : Component<...>
{

    protected override void OnMounted()
    {
        var myService = Services.GetService<IMyService>();
        myService.NewMessage += OnNewMessage;
    }

    protected override void OnWillUnmount()
    {
        var myService = Services.GetService<IMyService>();
        myService.NewMessage -= OnNewMessage;
    }

    void OnNewMessage(object sender, EventArgsWithMessage args)
  {
             switch (args.Message.Operations)
            {
                case Operations.Something:
                {
                    //small perf tip: use only one SetState
                    SetState(_ => 
                   {   
                         _.Responses?.Add(new(Operations.Something, response)));
                          if (response.Status == OperationStatus.Success && response.Result is JObject jObject)
                          {
                              _.SomethingResult = jObject.ToObject<Something>());
                          }
                    });

                    break;
                }
            }
  }
}
adospace commented 11 months ago

Hi, I was able to reproduce the issue using this code, working to fix it:

public class NavigationMainPageState
{
    public int Value { get; set; }
}

public class NavigationMainPage : Component<NavigationMainPageState>
{
    protected override void OnMounted()
    {
        System.Diagnostics.Debug.WriteLine("NavigationMainPage.OnMounted()");

        base.OnMounted();
    }

    protected override void OnWillUnmount()
    {
        System.Diagnostics.Debug.WriteLine("NavigationMainPage.OnWillUnmount()");

        base.OnWillUnmount();
    }

    public override VisualNode Render()
    {
        return new NavigationPage()
        {
            new ContentPage
            {
                new StackLayout()
                {
                    new Label($"Value: {State.Value}")
                        .AutomationId("MainPage_Label") //<-- required for sample test
                        ,
                    new Button("Move To Page")
                        .AutomationId("MoveToChildPage_Button") //<-- required for sample test
                        .OnClicked(OpenChildPage)
                }
                .VCenter()
                .HCenter()
            }
            .Title("Main Page")        
        };
    }

    private async void OpenChildPage()
    {
        if (Navigation == null)
        {
            return;
        }

        await Navigation.PushAsync<ChildPage, ChildPageProps>(_ =>
        {
            _.InitialValue = State.Value;
            _.OnValueSet = this.OnValueSetFromChildPage;
        });
    }

    private void OnValueSetFromChildPage(int newValue)
    {
        SetState(s => s.Value = newValue);
    }
}

public class ChildPageState
{
    public int Value { get; set; }
}

public class ChildPageProps
{
    public int InitialValue { get; set; }

    public Action<int>? OnValueSet { get; set; }
}

public class ChildPage : Component<ChildPageState, ChildPageProps>
{
    protected override void OnMounted()
    {
        State.Value = Props.InitialValue;
        System.Diagnostics.Debug.WriteLine("ChildPage.OnMounted()");

        Task.Run(async () =>
        {
            while (true)
            {
                await Task.Delay(1000);

                SetState(s => s.Value++);
            }
        });

        base.OnMounted();
    }

    protected override void OnWillUnmount()
    {
        System.Diagnostics.Debug.WriteLine("ChildPage.OnWillUnmount()");

        base.OnWillUnmount();
    }

    public override VisualNode Render()
    {
        return new ContentPage()
        {
            new StackLayout()
            {
                new Entry()
                    .AutomationId("ChildPage_Entry") //<-- required for sample test
                    .Text(State.Value.ToString())
                    .OnTextChanged(newText =>
                    {
                        if (int.TryParse(newText, out int value))
                        {
                            State.Value = value;
                        }
                    })
                    .Keyboard(Keyboard.Numeric),
                new Button("Back")
                    .AutomationId("MoveToMainPage_Button") //<-- required for sample test
                    .OnClicked(GoBack)
            }
            .VCenter()
            .HCenter()
        }
        .Title("Child Page");
    }

    private async void GoBack()
    {
        if (Navigation == null)
        {
            return;
        }

        Props.OnValueSet?.Invoke(State.Value);

        await Navigation.PopAsync();
    }
}
adospace commented 11 months ago

Fixed with version 1.0.147, please check it out