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

Generic JSException on aborting open/save dialogs #35

Open
janjanech opened this issue Jan 9, 2023 · 3 comments
Open

Generic JSException on aborting open/save dialogs #35

janjanech opened this issue Jan 9, 2023 · 3 comments

Comments

@janjanech
Copy link

When user aborts open/save dialog, showSaveFilePicker/showOpenFilePicker APIs throws AbortError which is unhandled by the Blazor.FileSystemAccess library.
The exception can be catched on the C# side only as generic JSException which has no mean to check what caused the error.
Please propagate AbortError to C# by using specialized exception or null return value.

@KristofferStrube
Copy link
Owner

Hey Jan,
Thanks for the issue. Sounds like a great idea.

There are multiple solutions for this as you state yourself.

I have stated and described a lot of options for solutions below and their usage. I welcome anyone to share their thoughts on each of these approaches so that we can arrive at the best solution.

Current solution

Just to restate what the current approach is: We don't handle errors in the library and leave the error handling to the user of the library as if they were making the JSInterop calls themself. The problem is as you state that you always get a JSException and that you don't have the option to distinguish between errors without parsing the exception message yourself.
This is what the consumption code looks like:

FileSystemFileHandle? fileHandle;

try
{
    fileHandle = await FileSystemAccessService.ShowOpenFilePickerAsync();
}
catch (JSException ex)
{
    // Handle Exception and notify the user of a generic error.
}

// At this point the fileHandle will not be null.

Return null on exception

We can make a try-catch around all the calls that state that they can throw an exception and return null if this happens. This is the easiest to implement but leaves the user with no knowledge about what actually caused the returned value to be null.
This is what the consumption code would look like:

FileSystemFileHandle? fileHandle = await FileSystemAccessService.ShowOpenFilePickerAsync();

if (fileHandle is null)
{
    // Notify the user of a generic error.
}
else
{
    // At this point the fileHandle will not be null.
}

Catch, Parse, and Throw

We could catch the JSException and either parse it or search for key parts of the message string and throw appropriate custom errors. This is i.e. what Chris Sainty does in Blazor/LocalStorage and other packages. One specific approach to this is here where he checks if the error message contains a specific sentence. This is nice, but it is not necessarily working in all browsers as that specific message is up to each browser implementer to formulate. (But again it is probably the same message in all major browsers.)
This is what the consumption code would look like:

FileSystemFileHandle? fileHandle;

try
{
    fileHandle = await FileSystemAccessService.ShowOpenFilePickerAsync();
}
catch (FatalErrorJSException fejsex)
{
    // Notify the user of a specific fatal error with some message.
}
catch (JSException jsex)
{
    // Handle Exception and notify the user of a generic error.
}

// At this point the fileHandle will not be null.

Catch the exception in JS.

For this, we would need wrapper JS methods for all methods that could throw specific errors. The question is what to do with this error. A general problem with this approach is that we would need to write many JS wrapper methods.

Callback methods.

We could do as David Pine has done in the Blazorators library and define two callback methods for either a successful invocation or an errored invocation. This complicates the code in the library and the code the consumer has to write. Another problem is that the code is very dissimilar to the corresponding JS code.
This is what the consumption code would look like:

FileSystemFileHandle? fileHandle;

await FileSystemAccessService.ShowOpenFilePickerAsync(
    successCallback: (handle) => fileHandle = handle,
    errorCallback: (error) => {
        switch (error) {
            case FatalError fe:
                // Notify the user of a specific fatal error with some message.
                break;
            default:
                // Notify the user of a generic error.
                break;
        };
    }
);

if (fileHandle is not null)
{
    // At this point the fileHandle will not be null.
}

Boxed returns

We can box the result of a method and on the returned object (which boxes the result) have a property that specified which error it is if any (string? type). Then create a custom exception type for each of these possible errors and throw them if it was an error. A problem with this is that there would be a constant overhead in every request that could throw an exception as we would need to unwrap the boxed response for every request.
This is what the consumption code would look like:

FileSystemFileHandle? fileHandle;

try
{
    fileHandle = await FileSystemAccessService.ShowOpenFilePickerAsync();
}
catch (FatalErrorJSException fejsex)
{
    // Notify the user of a specific fatal error with some message.
}
catch (JSException jsex)
{
    // Handle Exception and notify the user of a generic error.
}

// At this point the fileHandle will not be null.

Have ASP.NET handle JSInterop errors better

The ideal solution would be for ASP.NET to handle this as it is a common problem that any JSInterop invocation could throw an error for many reasons.

Standard JS Exception implementations.

They could do this by checking for the specific type of error when they invoke the method. The obvious counterargument for this would be that anyone can make specific errors in JS which Blazor couldn't possibly cover. My answer to this would be to at least cover the exception types specified here in the WebIDL standard specs and to have specific exception types for the error codes that a DomException can have. An example could be AbortErrorDomJSException.

Closing comments.

I feel like this needs some discussion before I settle on any solution and welcome anyone to share their thoughts.

@KristofferStrube
Copy link
Owner

I have discussed this on other channels and have concluded that most like the Catch, Parse, and Throw approach.

I will look more into how we can create some systematic exceptions that can potentially be used in multiple scenarios.

@KristofferStrube
Copy link
Owner

For people following this issue you might be interested in the latest release of Blazor.WebIDL and its accompanying Twitter post: https://twitter.com/KStrubeG/status/1656162148182028289

exceptions-web-idl.mp4

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

No branches or pull requests

2 participants