oqtane / oqtane.framework

CMS & Application Framework for Blazor & .NET MAUI
http://www.oqtane.org
MIT License
1.89k stars 546 forks source link

INotifyPropertyChanged #3593

Closed GOstSRB closed 9 months ago

GOstSRB commented 11 months ago

Greeting, I noticed a problem with INotifyPropertyChanged. In this example that is on the blog https://www.oqtane.org/blog/!/40/inotifypropertychanged. When a Module using this does its not work properly. It works correctly only when the browser is refreshed and does not change the page. If the page is changed from nav menu several times to other pages and when we return to the page where the module "Subscriber component" which uses INotifyPropertyChanged method PropertyChanged(object sender, PropertyChangedEventArgs e). This method is run 2 - 5 times.

sbwalker commented 10 months ago

@GOstSRB are you reporting that the subscriber never receives the publishers event... or that the the subscriber receives the publishers event multiple times?

GOstSRB commented 10 months ago

Receive multiple times.

GOstSRB commented 10 months ago

https://github.com/oqtane/oqtane.framework/assets/51758459/0b5f57b5-7901-4118-9674-22dedeeea4ad

sbwalker commented 10 months ago

@GOstSRB just to clarify... the code on the https://www.oqtane.org/blog/!/40/inotifypropertychanged blog includes a Publisher event on the OnInitialized method of a component. This means that every time that component is rendered on a page it will raise a new PropertyChanged event. The video you posted shows the method where the Subscriber receives events - but does not show the component which is raising the events. If the component raising the event is on the same page as the component receiving the event then each time you navigate to that page, the PropertyChanged event breakpoint will be invoked.

GOstSRB commented 10 months ago

This problem is easy to reproduced.

  1. In the video I posted is a fresh Oqtane installation. I was working with Oqtane version 3.4.3. I believe this happens in all versions.
  2. I created a new module and implemented everything from the blog into that newly created module. Only on page Modul was added newly created module.

And that is it. After that, if the browser is refreshed, everything is ok, it only enters once. If it is changed by pages as shown in the video, it enters 7 times. The component Edit.razor in video is rendered 2 times.

Component Index.razor

protected override void OnInitialized()
{
    ((INotifyPropertyChanged)SiteState.Properties).PropertyChanged += PropertyChanged;
}
 public void Dispose()
 {
     ((INotifyPropertyChanged)SiteState.Properties).PropertyChanged -= PropertyChanged;
 }

private void PropertyChanged(object sender, PropertyChangedEventArgs e)
{
    if (e.PropertyName == nameof(MyProperty))
    {
        MyProperty = SiteState.Properties.MyProperty;
        StateHasChanged();
    }
}

Component Edit.razor protected override void OnInitialized() { SiteState.Properties.MyProperty = true; }

sbwalker commented 10 months ago

@GOstSRB this issue took me down a rabbit hole which resulted in PR #3611 - which is not really related to INotifyPropertyChanged. I am going to document my findings here for future benefit...

High level: Blazor is a SPA framework (the concept of a "single page" is very important to understand). Each component is a stateless chunk of UI on the page. State is communicated to components in various ways - each with their own specific behavior. Oqtane leverages a cascading parameter called PageState to pass state from the router to all nested components in the UI hierarchy. Whenever a cascading parameter is changed, all components which reference it are immediately notified (note this is a Blazor behavior - not an Oqtane behavior). This results in components being re-rendered to reflect the latest state. This all makes sense in theory... however in practice it behaves a bit different than expected.

In a default Oqtane installation it will display the Home "page" as part of the initial request (a "page" in Oqtane is simply a configuration which defines the modules which should be rendered dynamically). The site router constructs a PageState object which becomes a cascading parameter. The page contains 3 Html/Text modules and the Oqtane UI page rendering process will render 3 instances of the Html/Text Index.razor component. Each instance is related to a unique ModuleId so it is able to load the specific content for each module instance.

If you then click on the Private link in the nav menu, the site router will recognize that you are navigating to a new "page". It will construct a new PageState object... and as soon as the cascading parameter is modified it triggers a notification to all existing components that the state has changed. The 3 Html/Text module instances are active in the DOM at this point so they all receive the notification (despite the fact that they are not part of the Private page and will ultimately be disposed at the conclusion of the rendering process). Each component will take action on the notification and re-render itself.

At this point the Oqtane UI page rendering process will kick in and will determine that it needs to create 1 new module instance specific to the Private page. It will render this component and at the conclusion of the rendering process, Blazor will recognize that the 3 module instances from the Home page are no longer in the DOM and will dispose those components.

So based on what I described above, you can see how components which were rendered on the previous "page" will be re-rendered due to the cascading parameter behavior. This means that the rendering process is sometimes doing more work than necessary ie. it is re-rendering components prior to disposing them. You generally do not notice that this is happening because Blazor has a very efficient shadow DOM algorithm which only modifies the UI when there are changes. And since components are regenerating the exact same UI output you only see the final result. However, if you look more closely at the actual component workload you may see that a module is doing more work than necessary.

I have introduced #3611 which optimizes the page rendering process. Specifically it uses conditional logic to determine if a component which receives a cascading parameter notification based on PageState changes should actually re-render. This eliminates a lot of redundant processing as you navigate around a site. And even more impactful for performance, I added this logic to the Html/Text Index.razor component as it was making API service calls to retrieve content for module instances that will be disposed.

Now... in regards to INotifyPropertyChanged... the example code on the blog is probably not optimal. Generally you would only raise an event based on a specific action - not when a component is initialized. You also need to check the value of the PropertyName parameter to determine the event that is being raised - as the Oqtane Framework itself uses the events system for some functionality.

GOstSRB commented 10 months ago

Great, I noticed the problem but didn't know how to present the problem. I have done subscriber and publisher successfully. I tested it. In this case only problem is when component Edit.razor is rendered and someone smash the back in browser and then try to add or edit. Add this code in newly created module: Component subscriber - Index.razor

@using System.ComponentModel
@inject SiteState SiteState

@* I used ActionLink *@
<ActionLink OnClick="@(() => CreateSiteProperty("Add", ""))" Action="Add" Security="SecurityAccessLevel.Edit" ResourceKey="Add" />

<ActionLink OnClick="@(() => CreateSiteProperty("Edit", "id=" + context.MOD_MODULEId.ToString()))" Action="Edit" Parameters="@($"id=" + context.MOD_MODULEId.ToString())" ResourceKey="Edit" />

@code {
private Task CreateSiteProperty(string Action, string Parameters)
{
    //Raise INotifyPropertyChanged and go to component to do things you need
    ((INotifyPropertyChanged)SiteState.Properties).PropertyChanged += PropertyChanged;
    if (Action == "Add")
    {
        NavigationManager.NavigateTo(NavigateUrl(PageState.Page.Path + "/*/" + ModuleState.ModuleId + "/" + Action));
    }
    else
    {
        string urlForEdit = $"{PageState.Page.Path}/*/{ModuleState.ModuleId}/{Action}";           
        NavigationManager.NavigateTo(NavigateUrl(urlForEdit, Parameters));
    }
    return Task.CompletedTask;
}

private void PropertyChanged(object sender, PropertyChangedEventArgs e)
{        
    if (e.PropertyName == nameof(MyProperty))
    {
        MyProperty = SiteState.Properties.MyProperty;
        if(MyProperty)
        {
            //After Save (Add, Edit on Component Edit)
            NavigationManager.NavigateTo(NavigateUrl());
        }
        else
        {
            //After Cancel on Component Edit
            NavigationManager.NavigateTo(NavigateUrl());
        }
        StateHasChanged();  
        Dispose();
    }
}

public void Dispose()
{
    ((INotifyPropertyChanged)SiteState.Properties).PropertyChanged -= PropertyChanged;
}
}

Component publisher - Edit.razor

@inject SiteState SiteState

<button type="button" class="btn btn-success" @onclick="Save">@Localizer["Save"]</button>
<NavLink class="btn btn-secondary" href="@NavigateUrl()">@Localizer["Cancel"]</NavLink> @*Must change to call Dispose() INotifyPropertyChanged on component Index*@
<button type="button" class="btn btn-secondary" @onclick="Cancel">@Localizer["Cancel"]</button>

@code {

private async Task Cancel()
{
    SiteState.Properties.MyProperty = false;
}

private async Task Save()
{
    try
    {
        validated = true;
        var interop = new Oqtane.UI.Interop(JSRuntime);
        if (await interop.FormValid(form))
        {
            if (PageState.Action == "Add")
            {
                MOD_MODULE MOD_MODULE = new MOD_MODULE();
                MOD_MODULE.ModuleId = ModuleState.ModuleId;
                MOD_MODULE.Name = _name;
                MOD_MODULE = await MOD_MODULE Service.AddMOD_MODULEAsync(MOD_MODULE);
                await logger.LogInformation("MOD_MODULE Added {MOD_MODULE }", MOD_MODULE );
            }
            else
            {
                MOD_MODULE MOD_MODULE = await MOD_MODULE Service.GetMOD_MODULE Async(_id, ModuleState.ModuleId);
                MOD_MODULE = _name;
                await MOD_MODULE .UpdateMOD_MODULE Async(MOD_MODULE );
                await logger.LogInformation("MOD_MODULE  Updated {MOD_MODULE }", MOD_MODULE);
            }               
            SiteState.Properties.MyProperty = true;
        }
        else
        {
            AddModuleMessage(Localizer["Message.SaveValidation"], MessageType.Warning);
        }
    }
    catch (Exception ex)
    {
        await logger.LogError(ex, "Error Saving MOD_MODULE {Error}", ex.Message);
        AddModuleMessage(Localizer["Message.SaveError"], MessageType.Error);
    }
}
}
sbwalker commented 10 months ago

@GOstSRB I understand that you are experimenting with INotifyPropertyChanged however it is not clear what you are actually trying to accomplish with this feature. If you described your goal then it would be possible for others to make suggestions on how to solve it.

GOstSRB commented 10 months ago

I wanted to do something like next scenario.
For member who need update find him in autocomplete and create for him new bill than send that member to devces.

Index.razor

<Autocomplete></Autocomplete> //members
UpdateUser() //User who make a pay send him in to N devices

Membership.razor

CreateAndPrintFiscalizationBill() //if bill Fiscalization OK! Open bill in new browser tab and Print it. Call just one time Index.UpdateUser() and select member in autocomplete.

Basically everything works, the only thing that can happen is a double call on UpdateUser().

zyhfish commented 9 months ago

This is because you need to implement the IDisposable interface to let the dispose method being called when you leave the page, otherwise the event will be registered multiple times: image

sbwalker commented 9 months ago

@zyhfish what you are saying is that if the module does not include the @implements IDisposable statement, the Dispose method which is included will never be executed - correct?

zyhfish commented 9 months ago

Hi @sbwalker , yes, the Dispose method will be called after implement the IDisposable or IAsyncDisposable interface: https://blazor-university.com/components/component-lifecycles/.