Skip to content
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

Merged
merged 14 commits into from
Aug 31, 2021

Conversation

TanayParikh
Copy link
Contributor

@TanayParikh TanayParikh commented Aug 27, 2021

Fixes: #16632
Fixes: #35929

@TanayParikh TanayParikh requested review from Pilchie and a team as code owners August 27, 2021 00:14
@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label Aug 27, 2021
@TanayParikh
Copy link
Contributor Author

API Proposal: #35929

@TanayParikh
Copy link
Contributor Author

/ping @dotnet/aspnet-blazor-eng for review 😄

Copy link
Contributor

@pranavkm pranavkm left a 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/IJSVoidResult.cs Outdated Show resolved Hide resolved
src/JSInterop/Microsoft.JSInterop/src/IJSVoidResult.cs Outdated Show resolved Hide resolved
src/Shared/JSInterop/JSCallResultTypeHelper.cs Outdated Show resolved Hide resolved
src/JSInterop/Microsoft.JSInterop/src/IJSVoidResult.cs Outdated Show resolved Hide resolved
@TanayParikh TanayParikh enabled auto-merge (squash) August 30, 2021 23:19
@TanayParikh TanayParikh merged commit 05303c7 into main Aug 31, 2021
@TanayParikh TanayParikh deleted the taparik/invokeVoidReturnVal branch August 31, 2021 00:52
@ghost ghost added this to the 7.0-preview1 milestone Aug 31, 2021
@StevenRasmussen
Copy link

Based on the label applied it looks like we’re going to have to wait another year to get this fix?

@ghost
Copy link

ghost commented Aug 31, 2021

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.

@TanayParikh
Copy link
Contributor Author

Based on the label applied it looks like we’re going to have to wait another year to get this fix?

It'll be going into .NET 6 RC2. main is targeted towards .NET 7 hence the label, we just have to backport it from here.

@TanayParikh
Copy link
Contributor Author

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1184532941

@github-actions
Copy link
Contributor

@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);
Copy link

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 ??

Copy link
Contributor Author

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).

TanayParikh added a commit that referenced this pull request Aug 31, 2021
* 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
TanayParikh added a commit that referenced this pull request Aug 31, 2021
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent InvokeVoidAsync Return Value Serialization InvokeVoidAsync shouldn't serialize return value to .NET
6 participants