-
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
Expose BlazorWebView.TryDispatchAsync() API for access to scoped services #13425
Conversation
This PR involves a lot of copy/paste because the new Aside from the copy/paste to simply expose the new API to users, most of the changes are for samples/tests. |
await WebViewHelpers.WaitForControlDiv(bwvHandler.PlatformView, controlValueToWaitFor: "Static"); | ||
|
||
// Use BlazorWebView.TryDispatchAsync to access scoped services | ||
var calledWorkItem = await bwv.TryDispatchAsync(async services => |
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 is the "positive" test that shows that the new pattern works.
await WebViewHelpers.WaitForControlDiv(bwvHandler.PlatformView, controlValueToWaitFor: "Static"); | ||
|
||
// DON'T use BlazorWebView.Dispatch to access scoped services; instead, just try to get it directly (and fail) | ||
var jsInterop = bwv.Handler.MauiContext.Services.GetRequiredService<ExampleJsInterop>(); |
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 is the "negative" test that shows that the if we don't use the new pattern then it fails.
await InvokeOnMainThreadAsync(async () => | ||
{ | ||
// Try to dispatch before the MAUI handler is created | ||
var calledWorkItem = await bwv.TryDispatchAsync(_ => throw new NotImplementedException()); |
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.
I added one test just to show that we do sometimes return false
(meaning the dispatch did not go through). In this case it's the most trivial "fail" case because the BlazorWebView wasn't even started at all.
There are some far more complex "fail" cases that I couldn't figure out how to test, because they're mostly to guard against "race conditions" where a navigation happens exactly during the dispatch. One is better than none.
@@ -1,4 +1,4 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<Project Sdk="Microsoft.NET.Sdk.Razor"> |
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.
I made the test project be a proper "MAUI Blazor Hybrid" app so that static web assets would get pulled in. Previously no tests needed this, but in this PR I added tests that rely on JS Interop through a scoped service, and that uses a static JS file in an RCL. Without this CSPROJ change the static JS files wouldn't get pulled in.
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.
We can remove all the await that are not really doing anything with the returned value but fwd it again. That will make the generated code cleaner. Task.FromResult(bool) should do the trick in the null case.
For example:
return _webviewManager is null ? Task.FromResult(false) : _webviewManager.TryDispatchAsync(workItem);
Changing base branch to net8 because this change requires net8-only changes. |
…ices This makes the new WebViewManager.TryDispatchAsync() API available to users of BlazorWebView.
89d5236
to
748f195
Compare
The PR hasn't changed except for: Rebased on net8 branch (because this change requires a net8-only change from aspnetcore), and fixed one test that failed. Need 1 approver to merge. @dotnet/dotnet-maui-blazor-eng |
/// <param name="workItem">The action to call.</param> | ||
/// <returns>Returns a <see cref="Task"/> representing <c>true</c> if the <paramref name="workItem"/> was called, or <c>false</c> if it was not called because Blazor is not currently running.</returns> | ||
/// <exception cref="ArgumentNullException">Thrown if <paramref name="workItem"/> is <c>null</c>.</exception> | ||
public virtual Task<bool> TryDispatchAsync(Action<IServiceProvider> workItem) => throw new NotSupportedException(); |
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.
Are we going to have cases where you need to do async work here? We might as well take a Func<IServiceProvider, Task>
to begin with if we think we might end up there at some point?
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.
We discussed numerous options in this comment thread (hidden by default): dotnet/aspnetcore#46644 (review)
Though I didn't see any mention of the work item itself being async...
So I guess we'll have to see if anyone wants that version as well.
This makes the new WebViewManager.TryDispatchAsync() API available to users of BlazorWebView.
Description of Change
Note: This change depends on the following two PRs:
Issues Fixed
Fixes #5804