Skip to content

[HybridWebView] Bubble up exceptions in JS into .NET#27129

Merged
mattleibow merged 5 commits intomainfrom
dev/hybridwebview-js-exceptions
Jan 23, 2025
Merged

[HybridWebView] Bubble up exceptions in JS into .NET#27129
mattleibow merged 5 commits intomainfrom
dev/hybridwebview-js-exceptions

Conversation

@mattleibow
Copy link
Copy Markdown
Member

@mattleibow mattleibow commented Jan 14, 2025

Description of Change

Previously an exception in JS would just fail to return to .NET, leaving the tasks unfinished.

This PR makes sure to catch the exception in JS and then send it back to .NET where it is re-thrown as a .NET exception.

There is additional work to remove the task manager from the handler definition and move the logic into a new service that is registered. This will allow for better testing and/or a new handler that does not need to also use the internal interface.

By default there is no behavior change, but you can enable this AppContext Switch with one line of code in your app's MauiProgram.cs:

AppContext.SetSwitch("HybridWebView.InvokeJavaScriptThrowsExceptions", isEnabled: true);

Issues Fixed

Fixes #27097

Copilot AI review requested due to automatic review settings January 14, 2025 19:26
@mattleibow mattleibow requested a review from a team as a code owner January 14, 2025 19:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.

Files not reviewed (4)
  • src/Controls/tests/DeviceTests/Resources/Raw/HybridTestRoot/index.html: Language not supported
  • src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs: Evaluated as low risk
  • src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs: Evaluated as low risk
  • src/Core/src/Handlers/HybridWebView/HybridWebViewTaskManager.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)

src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs:115

  • Ensure that the new exception handling logic for '__InvokeJavaScriptFailed' is covered by tests.
case "__InvokeJavaScriptFailed":

src/Core/src/Handlers/HybridWebView/HybridWebViewTask.cs:5

  • [nitpick] The term 'TaskId' is clear, but ensure that all new symbols follow the same clarity and consistency.
internal record struct HybridWebViewTask(string TaskId, TaskCompletionSource<string?> TaskCompletionSource);

@mattleibow mattleibow self-assigned this Jan 14, 2025
@mattleibow mattleibow added the area-controls-hybridwebview HybridWebView control label Jan 14, 2025
@mattleibow mattleibow added this to the .NET 9 SR4 milestone Jan 14, 2025
@mattleibow mattleibow removed their assignment Jan 15, 2025
@mattleibow mattleibow changed the title Bubble up exceptions in JS into .NET [HybridWebView] Bubble up exceptions in JS into .NET Jan 15, 2025
@mattleibow mattleibow force-pushed the dev/hybridwebview-js-exceptions branch from 80447ea to 94d0e3b Compare January 15, 2025 13:58
Eilon
Eilon previously approved these changes Jan 15, 2025
Copy link
Copy Markdown
Contributor

@Eilon Eilon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code and tests look good to me. Please note my 2 comments.

@mattleibow
Copy link
Copy Markdown
Member Author

/rebase

@mattleibow
Copy link
Copy Markdown
Member Author

mattleibow commented Jan 17, 2025

This should be behind a flag. I need to create an issue for net10 to make the switch by default to throw.

@mattleibow mattleibow force-pushed the dev/hybridwebview-js-exceptions branch from 5b4193f to 1e26742 Compare January 17, 2025 18:01
@mattleibow mattleibow force-pushed the dev/hybridwebview-js-exceptions branch from 1e26742 to 628187f Compare January 21, 2025 14:35
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-controls-hybridwebview HybridWebView control

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

HybridWebView needs a consistent and usable story for handling JS methods that throw exceptions

4 participants