-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Blazor Streaming Interop | JS
to DotNet
#33491
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
Conversation
src/Components/Server/src/BlazorPack/BlazorPackHubProtocolWorker.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop.JS/src/src/Microsoft.JSInterop.ts
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Implementation/JSObjectReferenceJsonWorker.cs
Outdated
Show resolved
Hide resolved
This is looking really good, @TanayParikh! Looking forwards to seeing the tests. No need to handle the .NET-to-JS direction in the same PR. |
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.
Looks great
src/JSInterop/Microsoft.JSInterop/src/Implementation/JSDataReference.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Implementation/JSDataReference.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Implementation/JSDataReference.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Implementation/JSDataReference.cs
Outdated
Show resolved
Hide resolved
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 fantastic! I know it's been a long road to get here, but this is a super feature and the implementation looks excellent. Thanks for bearing with us through all the reviews.
I only found one minor detail to raise: https://github.com/dotnet/aspnetcore/pull/33491/files#r658898696, so please feel free to deal with this however you see fit and merge.
return false; | ||
} | ||
|
||
return await circuitHost.ReceiveJSDataChunk(streamId, chunkId, chunk, error); |
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.
Since reviews are wrapping up, just calling this out from an offline conversation, this await will block the entire Hub until the operation completes which affects user interactivity
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.
Just to clarify/confirm, "this await will block the entire Hub" is referring to blocking that specific circuit, or all circuits on the server? I was under the impression it was the former, but "entire Hub" made me want to double check.
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.
Sorry, just that connection
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.
Thanks for the clarification. I discussed this with @javiercn and we're acknowledging the blocking here, but are electing to keep it. This is because we're going to be blocking the circuit regardless when we call into circuitHost.ReceiveDataChunk
which calls for Renderer.Dispatcher.InvokeAsync
ensuring we're on the main circuit thread. This is done to ensure the server/client runs in the same synchronization context (as is done with all the other endpoints). Additionally the ReceiveJSDataChunk
is a quick process, it should return near instantly in most cases. Lastly, we're utilizing the return value as a heartbeat for the transfer process, and without it would likely need to setup a separate endpoint to handle that functionality.
Thanks all for the reviews! I appreciate all the time you took and the feedback provided. Merging this PR for now, with the following action items:
|
477f337
to
e38913d
Compare
e38913d
to
6e91d84
Compare
CI doesn't like the timeout related tests (passing locally), iterated through several different configs / potential issues. I suspect this is likely due to the time sensitive nature of the tests which doesn't really lend itself too well to CI. Creating a separate PR to add back in those tests. |
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
API review feedback: Remove the pause and resume parameters: ValueTask<Stream> OpenReadStreamAsync(long maxAllowedSize = 512000, CancellationToken cancellationToken = default); |
Streaming interop mechanism from JS to DotNet
API
[Code]
Usage
JS
DotNet
Upload 100MB
Cancel Upload
Upload Files
Image Upload