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

[Bug]: Exception logged for tooltip module #5175

Closed
njannink opened this issue Dec 5, 2023 · 11 comments · Fixed by #5611
Closed

[Bug]: Exception logged for tooltip module #5175

njannink opened this issue Dec 5, 2023 · 11 comments · Fixed by #5611
Assignees
Labels
Type: Bug 🐞 Something isn't working

Comments

@njannink
Copy link
Contributor

njannink commented Dec 5, 2023

Blazorise Version

1.3.3

What Blazorise provider are you running on?

Material

Link to minimal reproduction, or a simple code snippet

From time to time an exception pops up in our application logs around the tooltip module. Probably the exception handling of loading stopped etc should be added

Steps to reproduce

What is expected?

What is actually happening?

Microsoft.JSInterop.JSException: Failed to fetch dynamically imported module: https://xxx/_content/Blazorise.Material/tooltip.js?v=1.3.3.0
TypeError: Failed to fetch dynamically imported module: https://xxx/_content/Blazorise.Material/tooltip.js?v=1.3.3.0
at Microsoft.JSInterop.JSRuntime.InvokeAsync[TValue](Int64 targetInstanceId, String identifier, Object[] args)
at Blazorise.Modules.BaseJSModule.InvokeSafeVoidAsync(String identifier, Object[] args)
at Blazorise.Tooltip.b__11_0()
at Blazorise.BaseAfterRenderComponent.OnAfterRenderAsync(Boolean firstRender)
at Blazorise.BaseComponent.OnAfterRenderAsync(Boolean firstRender)
at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle, ComponentState owningComponentState)

What browsers are you seeing the problem on?

No response

Any additional comments?

No response

@njannink njannink added the Type: Bug 🐞 Something isn't working label Dec 5, 2023
@stsrki
Copy link
Collaborator

stsrki commented Feb 13, 2024

@David-Moreira Do you think it might be worth catching a JSException along with other exception types?

@David-Moreira
Copy link
Contributor

Failed to fetch dynamically imported module

When Microsoft.JSInterop.JSException + Failed to fetch dynamically imported module
I don't know if there are actual JSException that we should let propagate, so better just handle specifically, in my opinion.

@David-Moreira
Copy link
Contributor

Or upon further inspection of the stack trace, the method is named InvokeSafeVoidAsync. I don't remember how we use it, but it would imply, that it's completly safe and would catch anything actually?

@stsrki
Copy link
Collaborator

stsrki commented Feb 13, 2024

We catch other exceptions:

protected async ValueTask InvokeSafeVoidAsync( string identifier, params object[] args )
{
    try
    {
        var module = await Module;

        if ( AsyncDisposed )
        {
            return;
        }

        await module.InvokeVoidAsync( identifier, args );
    }
    catch ( Exception exc ) when ( exc is JSDisconnectedException or ObjectDisposedException or TaskCanceledException )
    {
    }
}

@David-Moreira
Copy link
Contributor

Should we catch all is my point? Otherwise, is it ever truly safe?

@David-Moreira
Copy link
Contributor

Do our js InvokeSafeVoidAsync calls ever need to propagate any exception to the end user?

@stsrki
Copy link
Collaborator

stsrki commented Feb 13, 2024

Do our js InvokeSafeVoidAsync calls ever need to propagate any exception to the end user?

That's a valid question. IMO, if we propagate it down to the user, then what is the point of catching exceptions?

@David-Moreira
Copy link
Contributor

That's right. But I think in general in Blazorise we take the approach of no exceptions.

I.e, let's say a user missconfigured a component, we don't throw an Exception telling him Parameter X is missing or the configuration is invalid.

So here, I'd probably take the same approach as it's internal code that does not need user interaction, modules are imbued/self contained in the library so there's no user setup with these?
Only thing that can sometimes happen is users firewalls blocking resources, but that should still be somewhat evident when things don't work at all and the resources are blocked on the network tab?

@stsrki
Copy link
Collaborator

stsrki commented Feb 13, 2024

I agree. Since we are usually quiet about the internal error, we should do the same here. But to make sure this is fixed properly we should first reproduce the error on our side. From my testing, it is really hard to reproduce it.

Only thing that can sometimes happen is users firewalls blocking resources, but that should still be somewhat evident when things don't work at all and the resources are blocked on the network tab?

Yes, the 404 should show on the browser console for any non-found or blocked resources.

@njannink
Copy link
Contributor Author

Maybe you could use a ILogger in the catch clause of InvokeSafeVoidAsync and log the exception with a warning level or lower so users of Blazorise can still turn on logging to see if somehting is going wrong?

@stsrki
Copy link
Collaborator

stsrki commented Feb 14, 2024

That might be a good alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug 🐞 Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants