gaelj / BlazorCodeMirror6

Blazor wrapper for CodeMirror 6
MIT License
17 stars 1 forks source link

Microsoft.AspNetCore.Http v2.2.2 deprecated #118

Closed pm64 closed 7 months ago

pm64 commented 7 months ago

Hey @gaelj, love the recent updates! Just a heads up that the CodeMirror6 project references Microsoft.AspNetCore.Http v2.2.2, which is deprecated.

gaelj commented 7 months ago

Indeed :open_mouth: ! I had no idea.

Looks like I have to drop [Parameter] public Func<IFormFile, Task<string>>? UploadFile { get; set; }, as adding <FrameworkReference Include="Microsoft.AspNetCore.App" /> to the csproj is not compatible with <SupportedPlatform Include="browser-wasm" />.

[Parameter] public Func<IBrowserFile, Task<string>>? UploadBrowserFile { get; set; } will still be available though.

pm64 commented 7 months ago

@gaelj can this be conditional, to where SSR and SSB retain the existing functionality?

gaelj commented 7 months ago

At first glance it doesn't seem possible within a single project. I could make a second library specifically for blazor server with the additional parameter but is it worth it? Personally I'm consuming the library in blazor server projects which all use IBrowserFile. Please correct me if I am missing something.

pm64 commented 7 months ago

@gaelj if you don't mind, please give me a few hours to read up on this. My main use cases at the moment involve (1) MAUI Blazor hybrid, and (2) Blazor SSR (no interactivity). Just want to better understand the implications.

gaelj commented 7 months ago

Sure, the change is in it's own branch.

pm64 commented 7 months ago

@gaelj quick question.. when you said "Personally I'm consuming the library in blazor server projects which all use IBrowserFile" did you mean Blazor WASM?

As Blazor grows this gets more confusing. But currently in .NET 8 there are 4 runtime scenarios: Blazor SSR, Blazor Server, Blazor WASM, and Blazor Hybrid (aka MAUI Blazor). I'm just trying to understand which scenarios are impacted, sorry for delaying resolution here.

gaelj commented 7 months ago

I meant the "old" Asp.Net Blazor Server template (prior to net8), when life was simple and we only had the choice between server and wasm. I have very little experience with MAUI, and the dotnet8 templates are not compatible with our custom auth backend so we can't use them (yet). IBrowserFile definitely works in this mode and in the "old" wasm mode. Regarding the new modes, I would have to check for each of them but I think static SSR might be incompatible. I expect the others (server and wasm interactive modes) to work fine. Regarding MAUI, IBrowserFile seems to be the way to go.

pm64 commented 7 months ago

Thanks @gaelj, I'm testing SSR now, will report back here with my findings.

pm64 commented 7 months ago

Well, the component doesn't seem to work with SSR (interactivity mode "none") currently -- the placeholder animations just run forever, no error in the console.

The component renders fine in MAUI Blazor, but because of https://github.com/microsoft/microsoft-ui-xaml/issues/7366 I can't do the drag-and-drop to see which upload handler is fired. I suspect the MAUI Blazor fix will make its way to us soon, but it's reasonable to assume IBrowserFile is used in that scenario.

Anyway, go for it! If it creates problems in the future we can always revisit.

gaelj commented 7 months ago

You can upload files either by drag & drop or by copy & paste. Maybe you can check in MAUI in this way. For SSR, Code Mirror needs to be able to modify the head of the page to inject its style sheets, so it needs HeadOutlet. After a quick check in MS docs it seems this is not supported in static SSR, as they only mention interactive modes.

pm64 commented 7 months ago

Hi @gaelj, thanks for the tip about copy/paste. I used that approach to test in MAUI, and unfortunately in this case IFormFile is used. Not sure if I mentioned that MAUI is my main target :\

To avoid adding a new project, could we make the reference conditional in the project file (using a Condition attribute on an ItemGroup node), then use compiler directives in the code to only include support for IFormFile when appropriate?

gaelj commented 7 months ago

Take a look at this bc0d4f96022c6622b5da793fa2001fc365725d48 - dooes it help ?

pm64 commented 7 months ago

Hey @gaelj I just read through this commit a few times. I see you've added a controller to handle the upload in the Blazor Server example.

I think I see what you're doing, but I don't think this solves the issue for MAUI, since there's no ASP.NET Core in that scenario.

But maybe I've misunderstood? I'm just now reading through CodeMirror6WrapperInternal.razor.JsInvokables.cs and getting a grasp on how the upload is actually handled.

gaelj commented 7 months ago

Sorry for being a bit vague earlier.

Ignore the controller, it exists for testing purposes only. The point is, I made a converter from IBRowserFile to IFormFile. It is used at the bottom of Examples.BlazorServer/Pages/Example2.razor:

private async Task<string> UploadBrowserFile(IBrowserFile file)
{
    var formFile = file.ConvertToFormFile();

It is made up of Examples.BlazorServer/BrowserFileStream.cs and Examples.BlazorServer/FileConverter.cs.

Can you try in MAUI with those ? You'll need to copy those 2 files and adapt the namespaces. If it works, I could package them in a dedicated Nuget package if you want.

pm64 commented 7 months ago

Thanks @gaelj for the explanation. I'm afraid I may have led you down a bad path. My initial assessment of MAUI's behavior was too naive -- I simply assigned handlers to both UploadFile and UploadBrowserFile, and observed that UploadFile was called at paste time. Now that I've carefully read how the upload is handled in CodeMirror6WrapperInternal.razor.JsInvokables.cs, I can see this observation is not relevant.

So before testing your commit, I wanted to go back and re-assess the MAUI situation, using the code from prior to your commit. Here's what I found:

When only UploadBrowserFile is assigned (and not UploadFile), the UploadBrowserFile handler is called at paste time. (So, right off the bat I can see we're probably on a wild goose chase.) In this case, if the clipboard contains raw image data, the rendered image is inserted into the document as expected. But if an actual file is pasted (for example, if copied from Explorer or perhaps Finder), then base64 text is inserted.

When only UploadFile is assigned (and not UploadBrowserFile), UploadFileFromJS in CodeMirror6WrapperInternal.razor.JsInvokables.cs is not even called at paste time, let alone the assigned handler. I don't understand this behavior, I would expect UploadFileFromJs to get called regardless of which handlers are assigned on the component.

When both UploadBrowserFile and UploadFile are assigned, the UploadFile handler is called (as expected, based on UploadFileFromJS). In this case, base64 text -- not the rendered image --- is inserted into the document, regardless of whether the clipboard contains raw image data or an actual file.

I still wanted to test https://github.com/gaelj/BlazorCodeMirror6/commit/bc0d4f96022c6622b5da793fa2001fc365725d48 for completeness, but I think I'm missing something here too. Involving IFormFile necessitates a reference to the deprecated Microsoft.AspNetCore.Http. I guess the deprecation issue and the WASM compatibility issue are two separate issues.

But strangely, versions 2.1.22 (September 2020) and 2.1.34 (May 2022) AREN'T deprecated:

image

Maybe the reason for these exclusions is apparent from https://github.com/dotnet/announcements/issues/217 (they seem to precede AspNetCore 2.2, so not sure?)

I'll revisit tomorrow after some coffee and try to reach a definitive conclusion. Thanks for hanging with me through this.

gaelj commented 7 months ago

What is your goal ? To upload the file to a remote server or simply to include its encoded contents in the document ?

The way the upload file works in JS, is that it invokes dotnet to process (for example upload to a remote server) the file and return an URL to this file. This process is up to you to implement. In the original example, the process does not actually upload to any remote server. Instead, it converts the file contents to base64, which is then used to craft a special URL that actually contains the file. This URL is passed back to the JS, which then adds it to the document in MD format. If the content type of the uploaded file is an image, the MD link will be an image type link and the image preview plugin will show its preview. But if you move the cursor next to the image (or click the image) you will see the contents of the base64 "image link". Although this works and was an easy way for me to demonstrate file upload without requiring a back-end server in the demo site, I wouldn't recommend using it in a production app. Uploading the file and adding a normal link to it in the MD in much cleaner. Also I noticed the line numbering gutter tends to get buggy whit very long links when line wrapping is enabled - I haven't found a way around this issue yet.

In the latest example, I added an endpoint to the Blazor Server project to demonstrate how to upload the file to a remote server. In this case instead of returning an URL I just return a string containing the file name and size, that proves the file was successively received by the server. A real implementation would need to return an actual link to the uploaded file.

The Microsoft.AspNetCore.Http Nuget package should not be used any more. I'm not sure why some older versions are not flagged but I still wouldn't use them in a new project. It is now either included in the SDK (for example in Microsoft.NET.Sdk.Web) or if not, you can enable it with <FrameworkReference Include="Microsoft.AspNetCore.App" />, for example in SDK Microsoft.NET.Sdk.Razor. This is not supported on the WASM platform.

The fact that UploadFile on its own did nothing is indeed a regression introduced by f3ec4156f5e07c8f3026e67e1de9330a746b981c . Maybe try with version 0.4.3 and don't assign anything to UploadBrowserFile if you want to check properly. Sorry for that.

Anyhow, going back to the latest version, you can try to copy BrowserFileStream.cs and FileConverter.cs in your MAUI project. And then in your UploadBrowserFile callback you can use the .ConvertToFormFile() extension method to convert the IBrowserFile to an IFormFile.

I've investigated conditional inclusion in the csproj but I didn't find a way to created a Condition on which of the SupportedPlatforms in being used in the consumer project.

pm64 commented 7 months ago

Hey @gaelj, thanks for the details, this is becoming much clearer.

I'm interested in use cases where uploading to a remote server or directly encoding the data in the URL each may be appropriate.. but in the specific MAUI project that led me here, I'd want to save the image to the user's local file system.

That aside, I think I see the confusion here. MAUI hybrid apps target Microsoft.NET.Sdk.Razor, which itself does not include the Microsoft.AspNetCore.Http bits. So to use your converters, I'd need to install the deprecated package.

I'll go ahead and do the UploadFile test with 0.4.3 for completeness, as well as some more experiments in MAUI now that I have a better handle on things, and report back here.

UPDATE: I repeated the UploadFile-only test on 0.4.3 and the handler did indeed fire as expected.

UPDATE: Some clarification on the "paste causes base64 text to appear in document" phenomenon.. First, I love the option of encoding images directly in the document and hope support for this will continue. But what I'm observing is that if the image data is sufficiently large, the base64 text is displayed (in addition to the actual image), regardless of cursor position.

Small image example:

image

Larger image example (notice cursor position):

image

I also determined that this issue isn't related to whether the image is copied as a file vs raw image data, only the amount of data.

CONCLUSION: MAUI hybrid is not affected by removal of [Parameter] public Func<IFormFile, Task<string>>? UploadFile { get; set; }. The utility methods you wrote for converting between IFormFile and IBrowserFile could be a huge time saver for developers. I was going to recommend adding these to the library itself, but they're probably fine living in the example.

Sorry to pollute this thread with the base64 text thing, let me know if I should open a separate issue on that.

gaelj commented 7 months ago

Yes, please open another issue for the base64 url not being decorated. I suspect it's because its long enough that its end is beyond of the visible range. The base64 encoding thing was more of a proof of concept for the examples, but I could include it in the component easily enough.

Regarding the file converter, I'm leaning towards creating a dedicated Nuget package. What do you think?

Did you try to add in your MAUI csproj <FrameworkReference Include="Microsoft.AspNetCore.App" />? This will provide you the aspnet http stuff, unless its not supported on the maui platform similarly to the wasm case.

Now regarding the feature where you want the upload file to the user's device. I'm having issues to understand 😅 - this would be a download rather that an upload, wouldn't it? And where is the image/file coming from initially? All links already are decorated with a paperclip icon that the user can click to download the file.

pm64 commented 7 months ago

Hey @gaelj, regarding Microsoft.AspNetCore.App -- I don't think this can be combined with Microsoft.NET.Sdk.Razor. If I try adding that FrameworkReference to my project file, Visual Studio vomits:

image

So unless I'm missing something, I think MAUI is relegated to IBrowserFile, which is totally fine (but please correct me if I'm wrong!).

I'd enthusiastically back whichever way you go with the file converter -- but my inclination would be to keep that code in the examples, maybe with a reference to it in the README. It's a minor detail that doesn't seem to warrant its own Nuget package. But again, just my opinion.

Regarding the file download/upload/whatever -- in MAUI, the app is being served from and running on the local device. So a typical scenario might be that the user editing a local MD file, pastes image data into the file, then the image is saved in its own file alongside the MD file.

The most important thing we need to write the file to disk is the bytes, which we have. So this may already be largely or entirely supported. I haven't tested this though, and there are a few possible things I need to check:

Let me know if you foresee any trouble here. If you're ready, we can probably close this issue. I'll try implementing the local file save and can open new issues if needed. Thanks for your help!

gaelj commented 7 months ago

I've added support to generate data URLs from an uploaded file and insert it from within the JS, and fixed the long link formatting issue. As you probably know, JS to .Net interop is rather costly in terms of performance so I'd rather keep it in the JS part if anyone's going to us it in production. But you will still suffer the overhead of passing a potentially gigantic document back and forth with interop. Also it tends to rapidly break the websocket connection in Blazor Server because the bandwidth usage is so high. You also have the problem of the huge link text which is hardly ergonomic for an end-user. Anyway I've added it because it wasn't much work, but it should be used with caution. I'd still highly recommend writing the file somewhere (be it on a remote server or the local device) and inserting a hyperlink pointing to the file's location. (You will still get image previews if it's an image in that way). This SO post might help. Regarding your naming scheme for attached files, what comes to mind is to be careful to prevent overwrites due to identical file name with differing contents. You could for example add a file name prefix with a time stamp, or a hash.

pm64 commented 7 months ago

Hey @gaelj, agree 100% -- it's only practical to use data URLs in a few very specific scenarios, so appreciate your attention to this detail. Very nice option to have when appropriate.

I'll do some testing around that SO post and open new issues if any obstacles arise.

And hash/timestamp in filenames makes sense, thanks for the suggestion!