dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.4k stars 10k forks source link

CascadingValueSource<T> should automatically invoke NotifyChangedAsync for INotifyPropertyChanged values #53257

Closed kzu closed 9 months ago

kzu commented 9 months ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

The documentation on cascading values only mentions in passing that:

... all recipients are subscribed for update notifications, which are issued by calling NotifyChangedAsync

But there's no guidance on who should invoke that method and how to achieve that specially given two-way binding with such state.

This is contrary to the expectation in all other C# UI frameworks that typically leverage INotifyPropertyChanged to automatically notify any subscribers when changes are raised via the PropertyChanged event.

Describe the solution you'd like

Perhaps the constructor for CascadingValueSource<T> should detect whether the received value implements INotifyPropertyChanged and automatically subscribe to changes in that case, such as:

        if (value is INotifyPropertyChanged changed)
            changed.PropertyChanged += (sender, args) => this.NotifyChangedAsync();

Additional context

I have worked around this by creating my own factory for such value sources:

public static class CascadingValueSource
{
    public static CascadingValueSource<T> Create<T>(T value, bool isFixed)
    {
        var source = new CascadingValueSource<T>(value, isFixed);

        if (value is INotifyPropertyChanged changed)
            changed.PropertyChanged += (sender, args) => source.NotifyChangedAsync();

        return source;
    }
}

I find such factories for generics instances much more intuitive to use, turning the following:

new CascadingValueSource<NavMenuState>(new NavMenuState(), false)

to the slightly more concise:

CascadingValueSource.Create(new NavMenuState(), false)
javiercn commented 9 months ago

@kzu thanks for contacting us.

Cascading values already offer a mechanism to notify descendant components of changes. Changing the value is what triggers updates in other components, cascading values should not do anything with the value itself, that's up to users.

SteveSandersonMS commented 9 months ago

I have worked around this by creating my own factory for such value sources:

Great, that's the correct solution. Blazor isn't intended to give special treatment to INotifyPropertyChanged; the intention is that app developers can plug that in (or any other observability pattern of their choice). In practice it's extremely rare for people to want to do INotifyPropertyChanged with Blazor.

ghost commented 9 months ago

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

kzu commented 9 months ago

Given a CascadingValue<User>, there is no automatic updating of any other components when any one of them changes a property of the cascaded User instance. You must explicitly invoke the notify method, which you would need to remember to put everywhere, no?

kzu commented 9 months ago

The following example doesn't work as described:

// program.cs
builder.Services.AddCascadingValue(s => new CascadingValueSource<User>(new User(), false));
@rendermode InteractiveAuto

<h3>UserInfo</h3>

<p role="status">User clicks: @User.Clicks</p>

@code {
    [CascadingParameter] public required User User { get; set; }
}
@page "/counter"
@rendermode InteractiveAuto

<PageTitle>Counter</PageTitle>

<h1>Counter</h1>

<p role="status">Current count: @User.Clicks</p>

<button class="btn btn-primary" @onclick="IncrementCount">Click me</button>

<!-- let's see if two-way binding "fixes" this? -->
<input type="number" title="Clicks" @bind-value="User.Clicks">

<p>
    <UserInfo />
</p>

@code {
    [CascadingParameter] public required User User { get; set; }

    private void IncrementCount()
    {
        User.Clicks++;
    }
}

Neither clicking the button nor typing a value in the bound input, causes the UserInfo component to update its value of the Clicks property.

With the approach I laid out in this issue, implementing IPropertyChanged on the User class would be all that's needed for that to work.

Repro solution at https://github.com/kzu/CascadingValueNotification

Please consider reopening.

SteveSandersonMS commented 9 months ago

@kzu The Clicks property on your User type is just public int Clicks { get; set; }, so calling its setter doesn't trigger any notifications anywhere. If you want your CascadingValueSource to trigger update notifications, you need to call its NotifyChangedAsync method otherwise nothing will happen.

Since you want to use INotifyPropertyChanged, you might want to define some utility method that adapts INotifyPropertyChanged objects into CascadingValueSource so that INotifyPropertyChanged's PropertyChanged calls NotifyChangedAsync.

SteveSandersonMS commented 9 months ago

Oh actually I just saw in your repo that you already know this and have already implemented the relevant adapter: https://github.com/kzu/CascadingValueNotification/blob/main/CascadingValueNotification.Client/Program.cs#L44.

Is there still anything missing here?

kzu commented 9 months ago

What's missing is IMHO, for this to be supported OOB :). Which is the whole point of the issue ("*should automatically invoke...."). It currently doesn't and being INotifyPropertyChanged such an integral part of binding in every .NET UI framework from MS I can remember, I thought it would be good if it was a first-class citizen.

In other words, I think something like the following should probably be built-in:

public static class CascadingValueSource
{
    public static CascadingValueSource<T> CreateNotifying<T>(T value, bool isFixed = false) where T : INotifyPropertyChanged
    {
        var source = new CascadingValueSource<T>(value, isFixed);

        value.PropertyChanged += (sender, args) => source.NotifyChangedAsync();

        return source;
    }
}

I elaborated a bit more on my blog: https://www.cazzulino.com/cascading-blazor.html

Automagic in full display:

https://github.com/dotnet/aspnetcore/assets/169707/ca360972-9ed9-4e8d-bf22-d5e71f057b7f

SteveSandersonMS commented 9 months ago

Thanks for clarifying. We're not planning to couple Blazor to INotifyPropertyChanged in the forseeable future, but I'm glad you have a reasonably convenient solution for your own apps.

Pinox commented 9 months ago

@SteveSandersonMS must say it's a pity this does not have a higher priority as the .NET ecosystem has really powerful libraries like Sergio Pedri CommunityToolkit.Mvvm and also some of the reactive libraries like Dynamic data from Roland Pheasant. These libs with Blazor will be "pure magic".

https://github.com/reactivemarbles/DynamicData https://github.com/CommunityToolkit/dotnet

SteveSandersonMS commented 9 months ago

@Pinox We try to keep things flexible and be guided by real usage. Today it's perfectly possible to integrate INotifyPropertyChanged with Blazor, so if people want to do that they already can. If the usage level grew enough then we'd certainly consider shipping some kind of built-in integration library too.

Pinox commented 9 months ago

@SteveSandersonMS Yea I totally understand. From a pure .net prespective these 2 libs represent around 2.1 million downloads combined in their top downloaded versions. Therefor in Blazor perhaps not well used yet but I would say from a .net perspective the demand is well established.

sbwalker commented 8 months ago

@kzu does your solution function correctly in a mixed render mode environment ie. the app is static but you have some components which are interactive? I am having some difficulty with INotifyPropertyChanged in this configuration where the PropertyChanged event is being fired and the component is receiving the event, however the UI is not being updated to reflect the new content. Specifically this is happening in the App component when communicating between root components. For example if you wanted to update the page title from within an interactive component in the Routes render tree. The standard HeadOutlet is not working in this mixed render mode configuration... but neither is my INotifyPropertyChanged implementation (which works fine on global interactive Blazor).

kzu commented 8 months ago

If you need to update UIs from INPC, obviously those pieces need to be interactive, not static. But other than that, it should just work, yes.

sbwalker commented 8 months ago

Thank you @kzu. Basically I am looking for a way in a static rendered Blazor app to update page head elements from a static or interactive component - as HeadOutlet does not work (https://github.com/dotnet/aspnetcore/issues/50268) . This issue https://github.com/dotnet/aspnetcore/issues/49131 logged by @danroth27 describes the scenario and @javiercn indicates that a cascading parameter would be the solution. However based on your answer, it sounds like cascading parameters will not work in a static scenario?

kzu commented 8 months ago

Why wouldn't a cascading value work?