Skip to content

Address JS to DotNet byte[] Interop API Review Feedback #34328

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

TanayParikh
Copy link
Contributor

Fixes: #34327

long maximumIncomingBytes,
long signalRMaximumIncomingBytes,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change wasn't part of the API review, but I just felt this updated name could avoid future confusion (ex. clarifying it's not the max incoming bytes for the whole stream somehow).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great clarification. I probably would have thought it referred to the stream length before :)

@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Jul 14, 2021
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I was in the review that must have covered it, so was the conclusion that we shouldn't expose configurability over the Pipe options because the defaults are always good? If so, I'm certainly happy about that. If the reasoning or goal is something different from that, please let me know because I'd love to fully understand.

Approving assuming this is the case :)

@pranavkm
Copy link
Contributor

I don't think I was in the review that must have covered it, so was the conclusion that we shouldn't expose configurability over the Pipe options because the defaults are always good

The defaults are reasonable, any configuration here isn't directly visible because the abstraction the user works with is a Stream, and Kestrel felt that they made a mistake exposing the pipeoptions to users in the few places they did since it's very easy to misconfigure it.

@TanayParikh
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pranavkm pranavkm merged commit de63b91 into taparik/wasmWebViewJSToDotnetStreaming Jul 15, 2021
@pranavkm pranavkm deleted the taparik/updateJSDotNetAPIBasedOnAPIReview branch July 15, 2021 01:14
dougbu pushed a commit that referenced this pull request Jul 15, 2021
…3986)

* Prototype Large File Upload Support Blazor

* Blob.slice Implementation

* Allow WASM to keep working

* Update CircuitStreamingInterop.ts

* PR Feedback

* Cleanup

* WASM

* WebView

* Webview Remove SendJSDataStream

* Refactor Shared Streaming Interop Code TS

* WebView DotNetObjectReference Based ReceiveData

* BaseJSDataStream Refactor

* E2E tests WASM

* Unhandled Exception

* ReceiveJSDataChunk using DotNet Object Reference for WASM/WebView

* WASM Unmarshalled Interop Support

* E2E tests

* Update BaseJSDataStream.cs

* PR Feedback

* Update Web(View, Assembly) to match interop followup items

* PR Feedback

* Updated Release .js

* Create Microsoft.JSInterop.WebAssembly.WarningSuppressions.xml

* Updated WASM/WebView Pull Based Implementation

* InputFile Updated Implementation

* Cleanup

* PR Feedback

* Remove ReadRequest

* PR Feedback II

* Tests

* PR Feedback

* Address `JS` to `DotNet` byte[] Interop API Review Feedback (#34328)

* Address `JS` to `DotNet` byte[] Interop API Review Feedback

Fixes: #34327

* Update RemoteJSDataStreamTest.cs

* Update JSRuntimeTest.cs

* Final PR Feedback

* Fix WebView for large Files
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.

4 participants