Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing ErrorBoundary in ModuleInstance component #1815

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

PavelVsl
Copy link
Contributor

@sbwalker
Copy link
Member

sbwalker commented Nov 17, 2021

@chlupac this is a good enhancement... as it reduces the amount of code that a developer needs to write and it makes the application more resilient to unhandled exceptions. It is also using the new DynamicComponent introduced in .NET 6 which eliminates the need for more primitive methods such as OpenComponent() CloseComponent() etc...

If we consider a typical example of how error handling is currently being done in module razor components:

    protected override async Task OnInitializedAsync()
    {
        try
        {
            await Something();
        }
        catch (Exception ex)
        {
            await logger.LogError(ex, "Initialize Error {Error}", ex.Message);
            AddModuleMessage(Localizer["InitializeError"], MessageType.Error);
        }
    }

You will notice that errors are logged to the event log ( using logger ). This is very important so that there is a permanent record of the issue. I assume it would not be very difficult to add this.

In addition, the module message usually provides a user friendly message in regards to the event where it occurred ( and this message can be localized to another language ). For example it might say "Error Loading Announcements" or "Error Saving Document". This seems to be a bit more challenging to accomplish as the ErrorBoundary catches any error regardless of where it was thrown within a razor component and there does not appear to be a way to provide any friendly message. Instead it just says "Program error in module ClassName, AssemblyName" - which is not user friendly and does not provide any info about the specific event which caused it. At the very least the general message would have to be modified to not expose internal system information to end users and instead display "An unexpected error occurred". However this still does not solve the event info granularity issue. Perhaps the only way to achieve event info granularity is to still include try...catch in each event?

EDIT: After further thought I do not think it is important to have event info granularity for standard users - as the extra information is not really relevant to them ie. it does not matter if an error occurred during "module load" or "module save" - all the user needs to know is that an error occurred while they were interacting with the module. As long as the exception details are logged in the event log, a host user is able to get the full context, including the event where the exception occurred. Note that this PR does change the current UX behavior, as it displays the full exception details to the host user whereas currently only the minimal message is displayed and the host user needs to click on the View Details link to see the full exception details. The current UX behavior was chosen because the stack trace can sometimes be very lengthy and disruptive to the page layout. However I can also see the benefit of displaying the full exception in context so that you do not need to navigate elsewhere and break your flow.

@PavelVsl
Copy link
Contributor Author

  1. ErrorBoundary is the last instance to catch unhanded exceptions, This is a big step forward, as these errors used to be caught at the application level and a reload of the entire page was required for recovery. This was very confusing for users.
  2. Logging is very important for developers in this case, but I haven't fully tested it yet, so I haven't published it yet.
  3. The two previous errors of this module, however, are also not logged, this should be changed.
  4. For the average user, all that is needed is information that something has happened and that the module is not working as it should. For the time being, I have chosen to list the entire exception for the administrator. If it is logged, a standard error message will suffice.
  5. I don't think it's a substitute for a standard try-catch pattern, but in many cases it may be enough for simpler modules.

@sbwalker
Copy link
Member

I will merge this PR and make some modifications to it to add logging, etc...

@sbwalker sbwalker merged commit 59850f4 into oqtane:dev Nov 18, 2021
@PavelVsl PavelVsl deleted the ErrorBoundary branch November 18, 2021 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants