Blazorise 1.1.6 Datagrid not refreshing

[njannink]

After I updated to Blazorise 1.1.6 my datagrids stopped refreshing even when explicitly calling the Refresh method the rows are not updating. I had to rollback to 1.1.5

[David-Moreira]

hmm I just went through some of our demo examples can't find one not working. https://bootstrapdemo.blazorise.com/tests/datagrid/data/in-memory

Can you give us steps to reproduce or just plain code? Thanks.

[njannink]

I will have a look if I can reproduce it in an insolated project

[David-Moreira]

Thanks, appreciated for taking the time.

[njannink]

If I use this code in the standard Blazor WASM project you see the datagrid doesnt refresh. When sorting by clicking a header you see the object

@using Blazorise.DataGrid
@using System.Reactive.Linq

@page "/fetchdata"

<PageTitle>Weather forecast</PageTitle>

<h1>Weather forecast</h1>

<p>This component demonstrates fetching data from the server.</p>

<DataGrid Data="@forecasts">
    <DataGridColumn Field="@nameof(WeatherForecast.Date)" Caption="Date" />
    <DataGridColumn Field="@nameof(WeatherForecast.TemperatureC)" Caption="Temperature C" />
    <DataGridColumn Field="@nameof(WeatherForecast.TemperatureF)" Caption="Temperature F" />
</DataGrid>

@code {

    private List<WeatherForecast> forecasts = new()
    {
        new()
        {
            Date = DateOnly.FromDateTime(DateTime.Today.AddDays(-1)),
            TemperatureC = -1
        }
    };

    protected override void OnInitialized()
    {
        Observable.Interval(TimeSpan.FromSeconds(1))
            .Subscribe(t =>
            {
                Console.WriteLine($"Forecast {t}");
                forecasts.Add(new()
                    {
                        Date = DateOnly.FromDateTime(DateTime.Today.AddDays(t)),
                        TemperatureC = (int)t
                    });

                InvokeAsync(StateHasChanged);
            });
    }

    public class WeatherForecast
    {
        public DateOnly Date { get; set; }

        public int TemperatureC { get; set; }

        public string? Summary { get; set; }

        public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
    }
}

References:

    <PackageReference Include="Blazorise.DataGrid" Version="1.1.6" />
    <PackageReference Include="Blazorise.Bootstrap" Version="1.1.6" />
    <PackageReference Include="Blazorise.Icons.FontAwesome" Version="1.1.6" />
    <PackageReference Include="Microsoft.AspNetCore.Components.WebAssembly" Version="7.0.2" />
    <PackageReference Include="Microsoft.AspNetCore.Components.WebAssembly.DevServer" Version="7.0.2" PrivateAssets="all" />
    <PackageReference Include="System.Reactive" Version="5.0.0" />

[njannink]

In 1.1.5 the code above works

[njannink]

BlazoriseDataGrid.zip

[David-Moreira]

Ahh you're mutating the collection with the .Add. That does not register on SetParametersAsync which we moved the SetDirty to in an attempt to keep Parameters as auto properties like Blazor recommends. It should work by adding Reload(), which actually tells the Datagrid the data might be stale. The implementation for Refresh is just a regular StateHasChanged().

@stsrki what is your opinion on this? Think we should revert this approach?

[njannink]

this behavior mimicks my real implementation where I use an ObservableCollection (https://dynamic-data.org/) that is listening to server events

[David-Moreira]

Actually, What I'm saying isn't 100% right. The Parameter setter still gets called and as such SetParametersAsync still gets called. I made sure to actually debug now. The thing we did do differently is that we added a check before calling SetDirty():

            if ( parameters.TryGetValue<IEnumerable<TItem>>( nameof( Data ), out var paramData ) && !Data.AreEqual( paramData ) )
                SetDirty();

but since the collection is the same reference in memory, by calling .Add, it does not detect any changes now...

Whereas before we add SetDirty everytime the setter was registered, so it didn't care.

        [Parameter]
        public IEnumerable<TItem> Data
        {
            get { return data; }
            set
            {
                SetDirty();
                data = value;
            }
        }

I believe it might be best for us to revert the behaviour, @stsrki agreed?

[David-Moreira]

this behavior mimicks my real implementation where I use an ObservableCollection (https://dynamic-data.org/) that is listening to server events

yea cool, I honestly never used that Observable API seems awesome. Have you tried the Reload?

[njannink]

The Reload method seems to work in the demo project. What is the difference then between Refresh and Reload. The DataGrid API doesn't seem really clear then.

Does the Reload have any other side effects? What I don't like about this is that I now everywhere have to make an @ref="grid" to the datagrids, while in the past the StateHasChanged() was enough

[David-Moreira]

Shold not have any side effects. Same result as before, just one extra step.

Yes I believe we'll be reverting as there shouldn't be a significant improvement that justifies otherwise. Just want @stsrki's opinion on this, but I believe it would be best.

Again Refresh is just a regular,

[njannink]

that Refresh method doesnt really make any sense then on the API

[stsrki]

I think that we should still check the data. If data is not changed, then there is no reason for the reload or making it SetDirty();. Otherwise, it would refresh every time the Data parameter is received, and it will slow down the grid.

[njannink]

It's not that only the collection can change by adding or removing objects, but also a specific object itself could change. Also an object could have changed position within the collection.

So this check is not correct

if ( parameters.TryGetValue<IEnumerable<TItem>>( nameof( Data ), out var paramData ) && !Data.AreEqual( paramData ) )
                SetDirty();

You can compare 2 enumerables by using SequenceEqual so the check should be

if ( parameters.TryGetValue<IEnumerable<TItem>>( nameof( Data ), out var paramData ) && !Data.SequenceEqual( paramData ) )
                SetDirty();

[stsrki]

The AreEqual is already checking for SequenceEqual internally.

Your Data has a reference to the same object in memory. And since you mutate it directly, it will check the same object.

[njannink]

Ahh ok that explains it, but it's quite common practise to manipulate lists that you use in collection views so you simple cannot perform this check. Making a copy of the data would also be wrong memory-wise

[stsrki]

That is how the MS recommends working with Blazor. In a declarative way. So copying the list is the best way to proceed.

You can read more about it in https://github.com/dotnet/aspnetcore/issues/40867

---

I think in the future, we will need to consider adding support for the INotifyPropertyChanged.

[njannink]

I understand this and that is why I explicitly call StateHasChanged() which in this case does nothing. So now I need to call everywhere where I use DataGrid's explicitly the Reload() method which is forcing me also to make @ref references in the code which is lowering the matainability of my code. Tbh I really don't like this change and worse its a breaking change in a patch

[njannink]

And copying the list might also be a bad idea. If you have a big list of struct values/records all these objects will be copied aswell which might be a big memory strain

[njannink]

Maybe you can just copy only the actual subset of the collection that is displayed based on the sort and filtering etc and check that on equality

[stsrki]

I agree that it is, in a way, a breaking change. Maybe we should have consider it for the 1.2 release instead of a patch. But since it was essentially a non-supported scenario, it would break the code sooner or later.

Copying an array, as far as I know, is the advised way in almost any SPA. I remember all the copying a long time ago when I worked in the React ecosystem. Reducers, for example, work by copying data on each interaction.

---

I think in the future, we will need to consider adding support for the INotifyPropertyChanged.

What do you think of INotifyPropertyChanged? @David-Moreira

[njannink]

What would be great if also INotifyCollectionChanged is supported. Then also ObservableCollections can be used

[stsrki]

I haven't thought of ObservableCollection, but it is not a bad idea

if ( Data is ObservableCollection<TItem> collection )
{
    collection.CollectionChanged += Collection_CollectionChanged;
}

private void Collection_CollectionChanged( object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs e )
{
    InvokeAsync( () => Reload() );
}

[njannink]

not just ObservableCollection any object that implements INotifyCollectionChanged can be supported that way

[David-Moreira]

I agree that it is, in a way, a breaking change. Maybe we should have consider it for the 1.2 release instead of a patch. But since it was essentially a non-supported scenario, it would break the code sooner or later.

Copying an array, as far as I know, is the advised way in almost any SPA. I remember all the copying a long time ago when I worked in the React ecosystem. Reducers, for example, work by copying data on each interaction.

I think in the future, we will need to consider adding support for the INotifyPropertyChanged.

What do you think of INotifyPropertyChanged? @David-Moreira

Yes I'm advocating to revert because its a breaking change like @njannink mentioned. We can still follow best practices and introduce it on a major version instead, but maybe explore the options to make it easy for people like @njannink to adapt.

No idea about INotifyPropertyChanged I guess someone asked about it like a year ago. No real thoughts on it as I personally never used. But if its intuitive enough and helps with this problem why not.

[stsrki]

@David-Moreira, as we have discussed. We will revert the change to the 1.1.x version. The next step is to move the new behavior to v1.2 where we will document and explain the reasons for the change.

[njannink]

Thanks @stsrki and @David-Moreira

Lets think about a good design for this for 1.2(+) and possibly also check with somebody from Microsoft what is the best approach in this case. Maybe the IsDirty comparison behavior should even be configurable