Skip to content

feat: Added InputFile setup#700

Merged
linkdotnet merged 16 commits intobUnit-dev:mainfrom
linkdotnet:feature/input-file
May 9, 2022
Merged

feat: Added InputFile setup#700
linkdotnet merged 16 commits intobUnit-dev:mainfrom
linkdotnet:feature/input-file

Conversation

@linkdotnet
Copy link
Collaborator

@linkdotnet linkdotnet commented May 2, 2022

InputFile - Easy setup of uploaded files

Description

This PR relates directly to #317 (closes #317) and should give an easy setup for InputFile. In its current state there is no easy way of setting up an InputFile. This PR introduces two methods which makes it possible.

  1. AddInputFileSupport to setup the JavaScript part
  2. UploadFilesAsync to configure the files which will be returned by InputFile and also directly invoking the OnChange event.

Implementation

The implementation is straight forward. We just introduce a bunch of new types which are test doubles of the ones from the framework itself. I decided to go with an extension for IRenderedComponent<InputFile> because users can have multiple InputFiles in their code and they are used to call cut.Find("..."); anyway.

Usage

A typical test would look like this:

[Fact]
public void ShouldUploadFile()
{
    var cut = RenderComponent<ComponentWithInputFile>();
    var uploadFile = InputFileContent.CreateFromText("Hello World");

    cut.FindComponent<InputFile>().UploadFiles(uploadFile);

What is open? / discussion

Here are some points I want to discuss:

  • Right now, we have to call two methods to make it work. Without AddInputFileSupport we will get JavaScript Interop errors that the call was not set up in strict mode. I didn't want to mock that call (Blazor._internal.InputFile.init) by default. But we could. Via default we setup the JS call and let the user opt-out via an extension method. That might make more sense, because on average the user does not want to setup InputFile stuff on their own.
  • We might want to have that call synchronous to fit better to the current API. Simply speaking we can just await the internal call synchronously.
  • The used InputFileContent is always byte[]. I played around with two versions: string and byte[] and went with byte[] because it makes our code easier and pushes the responsibility to the caller instead of us. Text / encoding can be very hard. And especially when people want to upload images or so, byte[] seems the appropriate way.
  • It is hard to create a InputFile test in any of our test assemblies because that type was introduced with net5.0. Either we build one from scratch via BuildRenderTree(RenderTreeBuilder builder). If you have any tips, let me know.

Alternatives

I played around with the idea to replace or a given InputFile via our ComponentFactories with a test double. We might make this work as well, but it doesn't seem that much nice when it comes down to implementation details.

Info

This PR is only draft atm and meant as a base for discussions. Once we are aligned on the API design, xmldoc and tests will follow.

@egil
Copy link
Member

egil commented May 4, 2022

A few thoughts.

  1. It would preferable to just have the InputFile support enabled by default. Can we do this without breaking existing users that do this themselves already?
  2. If there is no issue with having a sync api, that is preferred.
  3. Have you considered having a private ctor on InputFileContent and then a bunch of Create methods that makes passing strings and other types in, as well as a byte[]? That way we can make it easy for most, and still allow for users that want special needs.
  4. I would create simple component in a stand alone project, and then copy the generated RenderTreeBuilder code into our sample project. We might also be able to simply call ctx.RenderComponent<InputFile>() inside a #if NET5_0_OR_GREATER.

@linkdotnet
Copy link
Collaborator Author

Perfect thank you very much for that valuable input ;) That helps a lot. I will update the PR and let you know once I have a presentable version.

@linkdotnet linkdotnet marked this pull request as ready for review May 7, 2022 16:22
@linkdotnet
Copy link
Collaborator Author

linkdotnet commented May 7, 2022

Hey @egil. Took some time today to work on that topic. Feel like I opened too much things and want to finish one by one those tasks.

The structure and everything is like it should be. I only have to satisfy some warnings in the test. (Edit: Fixed as well)

I basically incorporated all the feedback you had. Let me know your thoughts.

The only current downside is that (this is artificial and I don’t thing we will have users running into that), users can’t mock the internal InputFile call anymore so that it throws an exception (which they shouldn’t do in the first place).
They can't mock it because once we setup an invocation in the JSRuntime a second setup is just ignored for the same invocation target.

@linkdotnet
Copy link
Collaborator Author

One open point on my todo-list is documentation. Where do you think this would fit the best?

Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Good work on this.

  1. A few suggestions and questions related to the code.
  2. Yes, documentation is needed, otherwise your efforts will be wasted here, most wont notice it. A small page under "Test doubles" is my best suggestion with the current documentation struture.

@linkdotnet
Copy link
Collaborator Author

linkdotnet commented May 8, 2022

Added all the feedback plus documentation. As always thanks for the nice feedback.

Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Almost :)

@linkdotnet
Copy link
Collaborator Author

I do think we covered now everything. Thanks again

@egil
Copy link
Member

egil commented May 9, 2022

I do think we covered now everything. Thanks again

Indeed. Good work!

@linkdotnet linkdotnet merged commit 8c3ae90 into bUnit-dev:main May 9, 2022
egil added a commit that referenced this pull request May 19, 2022
* feat: Added InputFile setup

Co-authored-by: Egil Hansen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for Blazors <InputFile> component

2 participants