mrpmorris / Fluxor

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

NullReferenceException when refreshing the page manually #290

Closed IKIKN closed 2 years ago

IKIKN commented 2 years ago

Hi,

First of all, thanks for this amazing library.

I'm implementing a very simple scenario for Blazor Server, based on the tutorial, to query an API, and displaying the results (very similar to what you do with the WeatherForecast on the "Effects" section of the tutorial).

When launching my application, and accessing the razor page containing the list, it works well, but if I refresh the page manually (F5), I get a NullReferenceException (Object reference not set to an instance of an object).

Of course, I'm not expecting to have the state unchanged after the refresh, I understand that a manual refresh should restart everything from scratch, but I was expecting that the app would just fetch the data from the API again, and that "IsLoading" would be set to "true" in the meantime.

       @if(ArchiveTypeState.Value.IsLoading) {
            <div>Loading...</div>
        }
        else
        {
            foreach(var archiveType in ArchiveTypeState.Value.ArchiveTypes)
            {
                <div>@archiveType.Label</div>
            }
        }

   @code {
    [Inject]
    private IState<ArchiveTypeState> ArchiveTypeState { get; set; }

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

    protected override void OnInitialized()
    {
        base.OnInitialized();
        Dispatcher.Dispatch(new FetchDataAction());
    }
}

The NPE occurs because "ArchiveTypes" is null. And it reaches this section because "IsLoading" is false, because, even though "OnInitialized" has been called, it looks like the State was not updated in time, so that IsLoading is not set to true soon enough.

I suppose that I could initialize "IsLoading" to true by default, or checking if "ArchiveTypes" is not null before performing a foreach on it, but I feel like I shouldn't have to do that and that it probably doesn't work well because I did something wrong ? What would be the point of "IsLoading" if I can't be sure that its values hasn't been set soon enough?

Should this be happening? What is the best way to address this?

Thanks!

mrpmorris commented 2 years ago

Have the default value for that property as an empty collection.

Also, there is this pattern https://github.com/mrpmorris/Fluxor/blob/4eebae68ffe34c57ea5587ad193a4598c48b36e9/Tutorials/02-Blazor/02E-ActionSubscriber/FluxorBlazorWeb.ActionSubscriberTutorial/Client/Pages/Customers/Search.razor#L7

IKIKN commented 2 years ago

Thanks for your answer.

Your solution will indeed work, in the sense that there won't be any exception anymore, but it will display "No customers found" before displaying the list of customers, while a more elegant solution should display "Loading...", and not make the customer think, even for a short period of time, that there are no customers.

That's just my opinion, and I could be wrong of course, but I'm not 100% satisfied with this pattern.

mrpmorris commented 2 years ago

My solution shows Loading...

IKIKN commented 2 years ago

I meant, in the case of a refresh (F5), won't it show "No customers" before displaying the customers ? With the list being empty/null, and the "IsLoading" boolean being false.

mrpmorris commented 2 years ago

Could have a HasLoaded boolean on the state too to cater for that. It's not a Fluxor thing, but just a question of what states do you need to have to make your app work how you want it to.