-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Stop Serializing Return Values for InvokeVoidAsync
#35807
Conversation
…et/aspnetcore into taparik/invokeVoidReturnVal
API Proposal: #35929 |
/ping @dotnet/aspnet-blazor-eng for 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.
Dopeness
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/IJSVoidResult.cs
Outdated
Show resolved
Hide resolved
Based on the label applied it looks like we’re going to have to wait another year to get this fix? |
Hi @StevenRasmussen. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
It'll be going into .NET 6 RC2. |
/backport to release/6.0 |
Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1184532941 |
@TanayParikh backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Stop Serializing `InvokeVoidAsync` Return Value
Applying: Fix E2E Test
.git/rebase-apply/patch:56: trailing whitespace.
warning: 1 line adds whitespace errors.
warning: Cannot merge binary files: src/Components/Web.JS/dist/Release/blazor.webview.js (HEAD vs. Fix E2E Test)
warning: Cannot merge binary files: src/Components/Web.JS/dist/Release/blazor.server.js (HEAD vs. Fix E2E Test)
Using index info to reconstruct a base tree...
M src/Components/Web.JS/dist/Release/blazor.server.js
M src/Components/Web.JS/dist/Release/blazor.webview.js
Falling back to patching base and 3-way merge...
Auto-merging src/Components/Web.JS/dist/Release/blazor.webview.js
CONFLICT (content): Merge conflict in src/Components/Web.JS/dist/Release/blazor.webview.js
Auto-merging src/Components/Web.JS/dist/Release/blazor.server.js
CONFLICT (content): Merge conflict in src/Components/Web.JS/dist/Release/blazor.server.js
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Fix E2E Test
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@@ -28,7 +28,7 @@ public static async ValueTask InvokeVoidAsync(this IJSObjectReference jsObjectRe | |||
throw new ArgumentNullException(nameof(jsObjectReference)); | |||
} | |||
|
|||
await jsObjectReference.InvokeAsync<object>(identifier, args); | |||
await jsObjectReference.InvokeAsync<IJSVoidResult>(identifier, args); |
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.
If it's a void type result why do we need to keep it? also what's the difference between InvokeVoidAsync vs InvokeAsync ??
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.
If it's a void type result why do we need to keep it?
We still need to have JS return so that we know when to stop awaiting. We could build a separate pipeline which doesn't return any value but that'd add unnecessary complexity / increase long term maintenance.
also what's the difference between InvokeVoidAsync vs InvokeAsync ??
InvokeVoidAsync
is used for calls not requiring a return result, whereas InvokeAsync<T>
expects a generic T
result. Note, InvokeAsync<IJSVoidResult>
should not be used in developer code (it's for framework internal use).
* Stop Serializing `InvokeVoidAsync` Return Value * Fix E2E Test * Add JSObjectReferenceTest * Remove `IJSVoidResult` from the public API * release .js files * Revert "Remove `IJSVoidResult` from the public API" This reverts commit e754872. * Add monocrash to gitignore * Fix Tests * Update src/JSInterop/Microsoft.JSInterop/src/PublicAPI.Unshipped.txt * PR Feedback @pranavkm
* Stop Serializing `InvokeVoidAsync` Return Value * Fix E2E Test * Add JSObjectReferenceTest * Remove `IJSVoidResult` from the public API * release .js files * Revert "Remove `IJSVoidResult` from the public API" This reverts commit e754872. * Add monocrash to gitignore * Fix Tests * Update src/JSInterop/Microsoft.JSInterop/src/PublicAPI.Unshipped.txt * PR Feedback @pranavkm
Fixes: #16632
Fixes: #35929