Bubble up exceptions in JS into .NET #27129
Conversation
There was a problem hiding this comment.
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);
| if (errorType === 1) { // throw number | ||
| throw value; | ||
| } else if (errorType === 2) { // throw string | ||
| throw `String: ${value}`; | ||
| } else if (errorType === 3) { // throw Error | ||
| throw new Error(`Generic Error: ${value}`); | ||
| } else if (errorType === 4) { // throw runtime Error | ||
| undefined.toString(); | ||
| } |
There was a problem hiding this comment.
I think these are the main types of errors in JS?
| @@ -91,20 +91,49 @@ | |||
| } | |||
| }, | |||
|
|
|||
| "__InvokeJavaScript": function __InvokeJavaScript(taskId, methodName, args) { | |||
There was a problem hiding this comment.
These changes are backwards/forwards compatible. Before this PR, an error just never calls back into C#. All the changes are in the new function that catches exceptions and calls back into .NET. If you have the old JS, nothing will change.
| public partial class HybridWebViewHandler : IHybridWebViewHandler, IHybridWebViewTaskManager | ||
| public partial class HybridWebViewHandler : IHybridWebViewHandler |
There was a problem hiding this comment.
Moved the task manager into a separate service to help with testing ... which I haven't written yet. But also, allows a new handler to work without depending on this internal interface.
Description of Change
Issues Fixed
Fixes #27097