-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Blazor Byte Array Interop Support #33015
Conversation
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.cs
Outdated
Show resolved
Hide resolved
{ | ||
throw new ArgumentOutOfRangeException($"Element id '${id}' cannot be added to the byte arrays to be revived with length '${jsRuntime.ByteArraysToBeRevived.Count}'."); | ||
} | ||
else if (data.Length + jsRuntime.ByteArraysToBeRevivedByteLength > (31*1024)) // TODO; get this limit from SignalR somehow? |
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.
else if (data.Length + jsRuntime.ByteArraysToBeRevivedByteLength > (31*1024)) // TODO; get this limit from SignalR somehow? | |
else if (data.Length - 32k > jsRuntime.ByteArraysToBeRevivedByteLength) // TODO; get this limit from SignalR somehow? |
To avoid int overflows.
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 believe this should be:
else if (data.Length + jsRuntime.ByteArraysToBeRevivedByteLength > (31*1024)) // TODO; get this limit from SignalR somehow? | |
else if (32k - data.Length < jsRuntime.ByteArraysToBeRevivedByteLength) // TODO; get this limit from SignalR somehow? |
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
return Array.Empty<byte>(); | ||
} | ||
|
||
return bytes.Value.ToArray(); |
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.
Still doing ToArray
in place of passing through the ReadOnlySequence<byte>
.
Concerns with using the ReadOnlySequence
relate to the fact that it doesn't implicitly cast to byte[]
so when we're examining method parameters we have a type mistmatch.
Additionally, as Javier mentioned in the other PR we need to verify the lifecycle/ownership of the ReadOnlySequence
.
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.
Concerns with using the ReadOnlySequence relate to the fact that it doesn't implicitly cast to byte[] so when we're examining method parameters we have a type mistmatch.
I think you'd need to change the receiving code (ComponentHub.SupplyByteArray
) to accept a ReadOnlySequence<byte>
instead of a byte[]
, so there wouldn't be a type mismatch.
Agreed on verifying the ownership of the buffer before doing that though.
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.
Yes, lets keep it as is for the time being and then we can do a quick check and change afterwards.
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.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/Infrastructure/ByteArrayJsonConverter.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/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.cs
Outdated
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs
Outdated
Show resolved
Hide resolved
efc976d
to
ed1412b
Compare
ed1412b
to
f710bd0
Compare
Had to revert the ByteArrayJsonConverter back to a previous version (f710bd0) due to ongoing compatibility issues in WASM/CI. Not able to repro the issue locally, and tried logging/different solutions, but not too efficient given each attempt needs to be run through the entire CI pipeline to see if it works. f710bd0 follows the same pattern as the JS/DotnetObjectReferenceJsonConverter so I don't think this should be a big deal. |
...Assembly/WebAssembly/src/Microsoft.AspNetCore.Components.WebAssembly.WarningSuppressions.xml
Show resolved
Hide resolved
src/Components/test/testassets/BasicTestApp/InteropTest/ByteArrayInterop.cs
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/BasicTestApp/wwwroot/js/jsinteroptests.js
Show resolved
Hide resolved
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/ByteArrayJsonConverter.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 looks fantastic!
I have one small comment that needs clarification. Other than that, this is good to go!
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.
Good to go!
Breaking change announcement proposal: Byte Array Interop in .NET 6.0 Preview 6Labels:
SummaryBlazor Server & WebAssembly now supports optimized byte array interop which avoids encoding/decoding byte arrays into Base64, facilitating a more efficient interop process. Breaking ChangeReceiving Byte Arrays in JSfunction ReceivesByteArray(data)
{
// Previously data was a Base 64 encoded string representing the byte array
// 6.0 Preview 6 and beyond, it'll be a Uint8Array (no longer requires processing the Base 64 encoding)
} which can be invoked by the following C# code: var bytes = new byte[] { 1, 5, 7 };
await _jsRuntime.InvokeVoidAsync("ReceivesByteArray", bytes); Sending Byte Arrays from JSIf .NET is expecting a For example, if you have something like this: var bytes = await _jsRuntime.InvokeAsync<byte[]>("someJSMethodReturningAByteArray"); then you must provide a Before: function someJSMethodReturningAByteArray() {
const data = new Uint8Array([ 1, 2, 3 ]);
const base64EncodedData = btoa(String.fromCharCode.apply(null, data as unknown as number[]));
return base64EncodedData;
} After: function someJSMethodReturningAByteArray() {
const data = new Uint8Array([ 1, 2, 3 ]);
return data;
} |
Announcement looks great. Suggested tweaks:
Replace WASM with WebAssembly.
For context, can you also show the corresponding .NET code that invokes this?
Typo: "Base 64 encoding array" should be "Base64 encoded array" |
Thanks @SteveSandersonMS, it's official dotnet/announcements#187! |
@TanayParikh / @SteveSandersonMS ... The announcement isn't labeled (and placing "in .NET 6.0 Preview 6" in the title isn't normal and doesn't filter well), so it's a bit hidden at the moment. dotnet/announcements#187 |
Hi @guardrex. 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. |
Thanks for pointing that out @guardrex, I had tried adding labels but I don't have the adequate permissions. Do you happen to have the required permissions or know how I can get them? I'd like to add in the following labels.
Also, are you familiar with how the change goes from dotnet/announcements to https://docs.microsoft.com/dotnet/core/compatibility/6.0#aspnet-core? |
I don't have permission ... I'm just a docs 🐈. Open a new issue here on the PU repo. They prefer not to work from closed issues/PRs like this. WRT the breaking change doc ... submit a docs issue (and possibly PR) for it. It looks like the docs aren't autogenerated but actually worked up manually and then modified manually. I see dev PRs going into it on the blame ..... https://github.com/dotnet/docs/blame/main/docs/core/compatibility/6.0.md Use the This page button at the bottom of the topic to open the issue and go from there. |
Hi @guardrex. 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. |
Implements changes discussed in #32259.
Fixes: #21877