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

Blazor InputLargeTextArea #34856

Closed
wants to merge 33 commits into from
Closed

Conversation

TanayParikh
Copy link
Contributor

@TanayParikh TanayParikh commented Jul 29, 2021

Usage:

<InputLargeTextArea @ref="TextArea" OnChange="TextAreaChanged" />


@code {
  InputLargeTextArea TextArea;

  public async Task GetTextAsync()
  {
      var streamReader = await TextArea.GetTextAsync();
      GetTextResult = await streamReader.ReadToEndAsync();
      StateHasChanged();
  }

  public async Task SetTextAsync()
  {
      var memoryStream = new MemoryStream();
      var streamWriter = new StreamWriter(memoryStream);
      await streamWriter.WriteAsync(new string('c', 50_000));
      await streamWriter.FlushAsync();
      await TextArea.SetTextAsync(streamWriter);
  }

  public void TextAreaChanged(InputLargeTextAreaChangeEventArgs args)
  {
      // args.Length represents the new textarea value length
  }
}

Implementation Notes:
Follows the overall pattern we have with the InputFile Component, communicating with the client and relaying back events.

Fixes: #30291
API Proposal: #35007

@TanayParikh TanayParikh requested a review from a team as a code owner July 29, 2021 20:43
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jul 29, 2021
Update Release .js files
Update InputLargeTextAreaTest.cs

Update InputLargeTextAreaTest.cs
@TanayParikh TanayParikh force-pushed the taparik/largeTextAreaComponent branch from 531b629 to af4f4b7 Compare July 30, 2021 00:32
@TanayParikh
Copy link
Contributor Author

/ping @aspnet-blazor-eng, this is ready for review 😄

@TanayParikh TanayParikh added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 3, 2021
@ghost
Copy link

ghost commented Aug 3, 2021

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

@ghost ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 3, 2021
@TanayParikh TanayParikh linked an issue Aug 3, 2021 that may be closed by this pull request
@pranavkm
Copy link
Contributor

pranavkm commented Aug 3, 2021

The code changes are fairly straight forward, but I'd like to make sure this is the right API shape.

@TanayParikh TanayParikh requested a review from Pilchie as a code owner August 5, 2021 22:26
@TanayParikh TanayParikh changed the base branch from main to taparik/dotnetToJSStreaming August 5, 2021 22:26
@TanayParikh TanayParikh requested a review from a team August 5, 2021 22:40
@TanayParikh
Copy link
Contributor Author

This is now ready for review based on the new symmetric StreamReader/StreamWriter approach discussed in the API Proposal: #35007

Base automatically changed from taparik/dotnetToJSStreaming to main August 6, 2021 14:34
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.

The PR looks great.

{
// Special casing support for empty textareas. Due to security considerations
// 0 length streams/textareas aren't permitted from JS->.NET Streaming Interop.
if (jsException.InnerException is ArgumentOutOfRangeException)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we throw a more specific exception? Hard to say that ArgumentOutOfRange is specifically because of a 0-length stream?

Copy link
Contributor Author

@TanayParikh TanayParikh Aug 6, 2021

Choose a reason for hiding this comment

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

Hmm in the original context, I believe the ArgumentOutOfRange exception may make sense, however I see your point about it still being ambiguous with other areas where that exception may be thrown.

How about this:
https://github.com/dotnet/aspnetcore/pull/34856/files#diff-30d94608ca41ec93dcbe8eb2bb646257519e86080e81ed5c246a715a8782c4b9R94-R102

Alternatively, we can decorate the Exception.Data property with something more specific (though that may not necessarily be statically typed).

@TanayParikh TanayParikh requested a review from pranavkm August 6, 2021 19:48
Comment on lines +38 to +43
// public override async Task InitializeAsync()
// {
// // Since the tests share interactivity with the same text area, it's easiest for each
// // test to run in its own browser instance.
// await base.InitializeAsync(Guid.NewGuid().ToString());
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// public override async Task InitializeAsync()
// {
// // Since the tests share interactivity with the same text area, it's easiest for each
// // test to run in its own browser instance.
// await base.InitializeAsync(Guid.NewGuid().ToString());
// }
public override async Task InitializeAsync()
{
// Since the tests share interactivity with the same text area, it's easiest for each
// test to run in its own browser instance.
await base.InitializeAsync(Guid.NewGuid().ToString());
}

@@ -509,7 +509,20 @@ export module DotNet {
return value;
});

class DotNetStream {
export interface IDotNetStreamReference {
Copy link
Member

Choose a reason for hiding this comment

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

In Typescript it's idiomatic for interfaces not to have an I prefix. I suppose the difference vs C# is that it's very meaningful to think about runtime types in .NET but not so relevant in JS.

}

/// <summary>
/// The textarea element for which the event was raised.
Copy link
Member

@SteveSandersonMS SteveSandersonMS Aug 9, 2021

Choose a reason for hiding this comment

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

Suggested change
/// The textarea element for which the event was raised.
/// The <see cref="InputLargeTextArea" /> for which the event was raised.

/// <summary>
/// Gets the length of the textarea value.
/// </summary>
public int Length { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Should the type be long? Can it be > 2GB? I know that it would be pretty outrageous to have a 2GB textarea, but do we definitely want to preclude it?

@TanayParikh
Copy link
Contributor Author

There are concerns with API complexity from having to work with Stream{Reader/Writer} when users may be expecting to work with strings directly. Supporting strings directly isn't optimal either due to the (large) amount of allocations it would trigger.

Closing this PR in favor of docs/samples of a solution for users that encounter this issue.

@dougbu dougbu deleted the taparik/largeTextAreaComponent branch August 21, 2021 22:37
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
3 participants