Enh: ActionDialog support for Static mode

[mdmontesinos]

Issue Description

ActionDialog is currently not supported in Static rendering modules.

Steps to Reproduce

1. Create a new module using the default External template.
2. Change the render mode to static
3. Click on the Delete button and the dialog does not appear.

Expected Behavior

I should be able to trigger the dialog and delete a module item in static mode.

Actual Behavior

The button to trigger the dialog does not work because it relies on the onclick handler, and neither would work the internal confirm or cancel buttons for the same reason.

Possible Solution

Similar to Pager module, ActionDialog should be constructed conditionally based on render mode to support form with submit instead of onclick

[sbwalker]

@mdmontesinos I was hoping to be able to implement static rendering support without any requiring any changes to the consumer of the component - however this was not possible.

In #4031 I had to introduce a new Id parameter which is required when using static rendering on a page which contains multiple ActionDialog components (ie. a list of items). The Id parameter should contain a unique identifier for the item... usually a database identifier. This Id will be used to construct unique form names so that Blazor is able to submit user interactions to the correct form. Note that Blazor does not support dynamic form names - the name must be immutable across form submission events. Here is an example of the Id parameter:

<ActionDialog Header="Delete Page" Message="@string.Format(Localizer["Confirm.Page.Delete"], context.Name)" Action="Delete" Security="SecurityAccessLevel.Admin" Class="btn btn-danger" OnClick="@(async () => await DeletePage(context))" ResourceKey="DeletePage" Id="@context.PageId.ToString()" /><

Note that this enhancement is not fully functional. You can display a modal dialog and Cancel it. However when the Confirm button is selected, the callback is being made to the method specified in the OnClick parameter, however a general error is being thrown after the operation completes:

"Cannot access a disposed object."

I have not been able to determine the root cause of the error yet - nor figure out a way to suppress it.

[mdmontesinos]

@mdmontesinos I was hoping to be able to implement static rendering support without any requiring any changes to the consumer of the component - however this was not possible.

Perhaps adding an auto-generated Guid in the case that no Id is supplied as a parameter could work for "legacy" support? In this way, the parameter is truly optional and won't case issues for components already using it, but still offering the possibility of setting the custom Id for better practices.

Note that this enhancement is not fully functional. You can display a modal dialog and Cancel it. However when the Confirm button is selected, the callback is being made to the method specified in the OnClick parameter, however a general error is being thrown after the operation completes:

"Cannot access a disposed object."

I'll try the new version to test if I'm also getting the same error. I'll update as soon as I can.

And as always, thanks for the quick response.

[sbwalker]

@mdmontesinos my comment "Note that Blazor does not support dynamic form names - the name must be immutable across form submission events" means that the component cannot generate its own Guid - this is what I tried initially and it does not work. This is because when the component is first initialized it generates a Guid and then when the form is submitted another Guid is gnerated and the Guids do not match so the form input cannot be matched to the component. This is why the unique identifier must be passed in and based on an immutable value.

[thabaum]

@attribute [StreamRendering] I believe fixes the issue in the module template... ;)

Needs to be included in the module component.

I think this is a documentation issue for Oqtane.Docs if holds water.

[sbwalker]

@thabaum In order to test this you need to add the following code to the module's Index.razor:

public override string RenderMode => RenderModes.Static;

and modify the markup to include the Id parameter:

            <td><ActionDialog Header="Delete" Message="Are You Sure You Wish To Delete This Zyhfish?" Action="Delete" Security="SecurityAccessLevel.Edit" Class="btn btn-danger" OnClick="@(async () => await Delete(context))" ResourceKey="Delete" Id="@context.[YourModuleName]Id"  /></td>

Build the module and make sure the Site is set to Static render mode.

After the discussions I had at the MVP Summit with the Blazor Team I would recommend not adding StreamRendering attributes - but instead document the scenarios which it seems to solve. I was told that it has side effects where it can cause UI "flashing" because it is pushing multiple payloads of content to the browser.

[thabaum]

@sbwalker sorry it was a quick reaction as I was not getting the module popup, but now I do, but the delete button does not work in the dialog.

I was using @ModuleState.ModuleId.ToString() to get the id, but again the dialog in not responsive to delete.

[sbwalker]

ModuleId is not a unique identifier - as soon as you have 2 items in the module list the ID will be a duplicate which will result in Blazor failing to match the form submission (and possibly raising an exception).

[thabaum]

It looks like I am at the current issue.

[sbwalker]

Yes, that is the issue that I reported above.

[thabaum]

Well, it seems like it is deleted, disposed of, then calls this function.

Any reason this is a "Put" and not a "Delete", seems like the wrong action...

[sbwalker]

Sorry I do not understand your question. The ActionDialog is generic and is designed to allow a user to confirm an action before it is excuted. The action is implemented in your own component and could be an Add, Update, Delete, or even a Get. In the case of the default module template it is a Delete button and it calls the Delete method on the Service which calls the Delete method in the Controller and Delete method in the Repository. I am not sure why you think it is doing a PUT?

[thabaum]

My apologies, I meant post not put.

[thabaum]

@sbwalker PostJsonAsync is called on a delete, the module object is already deleted when this is called.

                                <form method="post" @formname="@($"ActionDialogConfirmForm{Id}")" @onsubmit="Confirm" data-enhance>
                                    <input type="hidden" name="__RequestVerificationToken" value="@SiteState.AntiForgeryToken" />
                                    <button type="submit" class="@Class">@((MarkupString)_iconSpan) @Text</button>
                                </form>

[sbwalker]

I have no idea what you are referring to. The ActionDialog does not call any HTTP methods. It simply calls whatever method was passed in on the OnClick parameter.

[sbwalker]

@thabaum this is the way Static Blazor forms work - you must POST back to the component which invokes the method specified with the OnSubmit attribute.

[thabaum]

This is from the post form method correct?

The "Confirm" button deletes the object, from what I can tell, I just get this error along the way.

[sbwalker]

No, I think the PostJsonAsync is being invoked from your own module code as a side effect - basically an error is being thrown and caught by the exception handler and then it is trying to Log the error - and it uses a Post to call the LogService. So I don;t think this is the cause.

[thabaum]

Everything works as expected, except the logger? This could be an issue with the logger as it should handle this without errors and log as a logger error?

[sbwalker]

No. The logger is designed to log errors which occur in components. An error has occurred in the component so it is going to try to log it. Suppressing the error is not the solution.

[sbwalker]

As soon as this line gets executed in the module component:

        await MyTestService.DeleteMyTestAsync(MyTestId, ModuleState.ModuleId);

An exception is thrown:

IOException: Unable to read data from the transport connection: The I/O operation has been aborted because of either a thread exit or an application request..
SocketException: The I/O operation has been aborted because of either a thread exit or an application request.

Which results in the exception handler catching the error and calling the logger:

            await logger.LogError(ex, "Error Deleting MyTest {MyTest} {Error}", MyTest, ex.Message);

Which then results in the PostJsonAsync exception. So the actual error is the thread abort exception... not the logger.

[thabaum]

Thanks for the clarification. I was not saying to suppress the error, but log the error of the logger.

1. The dialog appears on delete.
2. the confirm button will delete the entry, but bugs out with this current exception trying to be logged in the logger.
3. This potential logger error is occurring rendering the action dialog into a buggy state after OnClick() function is performed in Confirm() method.

    private void Confirm()
    {
        if (PageState.RenderMode == RenderModes.Interactive || ModuleState.RenderMode == RenderModes.Interactive)
        {
            DisplayModal();
        }
        OnClick();
    }

[sbwalker]

@thabaum I think the problem is that the HttpClient service is being disposed. And I think it has something to do with the differences in scope between static components and interactive components. Maybe using HttpClientFactory would help... but only if it does not affect the run-time characteristics or performance of Interactive Blazor. In general, I really want to minimize conditional logic based on render mode as much as possible... or else it will become a nightmare to maintain.

[thabaum]

@sbwalker HttpClientFactory seems to be the direction to go forward with and is designed to improve performance and scalability reusing existing and disposing of HttpClient instances to reduce overhead along with avoiding socket exhaustion. I believe this is worth exploring.

[sbwalker]

Yes it is worth exploring. I do find it ironic that there was so much criticism of Oqtane’s approach of having Interactive server components call server APIs - yet it allowed the framework to avoid so many complex process model problems by having an abstraction layer to manage these issues. Unfortunately Static rendering can’t seem to be solved in the same way :(

[thabaum]

I do find it ironic that there was so much criticism of Oqtane’s approach of having Interactive server components call server APIs

@sbwalker I believe what is very ironic... is by version 10 I think everything is API based but super efficient... like I said once I believe you are far out in the future man. We got to pull you back in time ;)

When this happens they will officially be able to call it "Blazor United" as Maui will be fully integrated if I am reading Microsoft correctly... it has been a few months since I reviewed things but I do recall this being discussed for a moment at some time.

[mdmontesinos]

Some updates now from my part.

In my use case, Static components will always be in this render mode, or perhaps Interactive Server, but never Interactive client. Therefore, I'm not using client-side services with HttpClient calls, but rather server-side services that directly use repositories.

In this situation, the ActionDialog works properly, the element is deleted, no exception is thrown and so pointing that the most probable cause is HttpClient.

Also, there's a bug that prevents the dialog from closing in Static mode after the OnClick action is executed (you probably haven't encountered it because an exception was thrown).

private void Confirm()
{
    if (PageState.RenderMode == RenderModes.Interactive || ModuleState.RenderMode == RenderModes.Interactive)
    {
        DisplayModal();
    }
    OnClick();
}

Should be replaced with:

private void Confirm()
{
    OnClick();
    DisplayModal();
}

The logic of closing the modal in Static/Interactive is already contained in DisplayModal, so it should always be used when confirming the action. Also, the action must be executed before because otherwise, the navigation in static mode cancels it.

[sbwalker]

@thabaum I implemented HttpClientFactory locally but it does not resolve this issue. It still throws the same error:

(plus I had to hardcode the BaseAddress for the HttpClientFactory as it cannot resolve the NavigationManager from the DI container - which makes this a dead end)

[thabaum]

@sbwalker thanks for the update.

[sbwalker]

further to above... I found out that when running on Static Blazor, there is a different version of NavigationManager which is used on the server: "Microsoft.AspNetCore.Components.Server.Circuits.RemoteNavigationManager". However when I try to get that service from the DI container I get:

Microsoft has made this a sealed internal class:

https://github.com/dotnet/aspnetcore/blob/29c9acc509e938a978f7d286c356af1796cfae54/src/Components/Server/src/Circuits/RemoteNavigationManager.cs#L15

[sbwalker]

I was able to figure out a workaround for the RemoteNavigationManager issue above... and I merged #4039 which adds HttpClientFactory support. In order to use it, you need to use a different constructor in your Service classes:

public PageService(IHttpClientFactory http, SiteState siteState) : base(http, siteState) { }

However, as I mentioned above, it does not resolve the "Cannot access a disposed object" issue which is the focus of this thread.

[thabaum]

@sbwalker I believe this is a great enhancement using HttpClientFactory, nice work!
public PageService(IHttpClientFactory http, SiteState siteState) : base(http, siteState) { }

I had like 25 or more of these I updated last night and changed back today to change directions :)

I am glad that still held water for an enhancement and was included in the framework, thank you.

I will have to roll out for a few now but back later full steam ahead by high noon my time or sooner. I read up the servicebase.cs file relating logic which interested me. I look forward to reviewing the code changes when I get back!

[sbwalker]

@mdmontesinos I added your change above which switched the order of OnClick and DisplayModal. And thank you for confirming the error is only occurring when using HttpClient. Based on that, we could simply have guidance that if you are using Static rendering in your component then you should not use HttpClient - you should use direct data access instead. I plan to update the module template today to follow the "dual service" approach which would effectively follow this guidance. Perhaps we can close this issue as resolved, as it does accomplish the goal of enabling the ActionDialog in Static rendering.

[mdmontesinos]

Glad to help!

And about the module template, I recently wanted to create server-side services for my static modules and was a bit lost on where to start. I ended up taking HtmlTextService as a reference because it includes both client and server versions. Also, I believe it's really important to have well documented module templates with both versions so that developers understand the differences in render modes. Especially taking into account the improvements in performance when not using HttpClient....

As for me, you can close the issue. The desired enhancement is already implemented, the HttpClient could be logged as a new issue.

[sbwalker]

@mdmontesinos I am working on the changes to the module template now and there are indeed a lot of changes if you want to use the dual service approach (which supports both Static and Interactive render modes). I was just testing the module using Static rendering and the ActionDialog still throws an exception related to a disposed object. However, I now suspect that the object which is disposed is actually the NavigationManager - not HttpClient. This is related to the same issue I already reported to Microsoft - and for which there is no workaround currently:

dotnet/aspnetcore#53996

By deselecting "Break when this exception type is user-handled" the operation works as expected. So I suspect this error is related to NavigationManager and only occurs when running in the Visual Studio debugger. And it probably is related to the fact that there are actually 2 different version of NavigationManager - one for Interactive and one for Static render modes.

[sbwalker]

@mdmontesinos #4042 contains all of the updates to the Module Template. I am going to close this issue.