-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Move HybridWebView platform code to handlers #24479
Conversation
@mattleibow / @tj-devel709 - CI is green, just awaiting your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semi-nitty, semi-not.
This code is looking much better, but a few changes will be needed to make the command be where all the action takes place. But, mostly there.
} | ||
|
||
|
||
public static async void MapInvokeJavaScriptAsync(IHybridWebViewHandler handler, IHybridWebView hybridWebView, object? arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PureWeen I think this needs that fakesync with the FireAndForget method - especially for logging?
This is a "pattern":
void MapSomething(IViewHandler handler, IView view, object args)
{
MapSomethingAsync(handler, view, args).FireAndForget(handler);
}
Task MapSomethingAsync(IViewHandler handler, IView view, object args)
{
// the real code
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course this means that either MapInvokeJavaScriptAsyncAsync
will exist or a MapInvokeJavaScriptAsyncInternal
:) Lovely, but secret lovely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be the FireAndForget system to make sure errors are logged and does not accidentally crash some iOS situations.
@mattleibow - I pushed an update based on our discussion. Please let me know if there's anything else necessary so we can merge. |
And run everything through the mappers. Fixes #24269
0fd5036
to
0992228
Compare
@mattleibow alright I think I've made all the suggested changes or replied with comments. Just waiting for CI to make the verdict. |
1e441c0
to
d0c58ec
Compare
* Revert "Move HybridWebView platform code to handlers (#24479)" This reverts commit 693e544. * - disable HybridWebViewTests --------- Co-authored-by: Shane Neuville <[email protected]>
And run everything through the mappers.
Fixes #24269