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

Markdown: ImageUploadChanged with multiple files fix #5975

Conversation

tesar-tech
Copy link
Collaborator

Description

Closes #5961

This effectively fixes the issue. The previous implementation overwrote the _blazorFilesById dictionary, which resulted in this error
when calling await file.WriteToStreamAsync(memoryStream);

The fix works, but there are plenty of areas for improvement.

Files One by One, Not Grouped

  • As @linkdotnet pointed out in the issue, files are processed one by one, even though FileChangedEventArgs supports multiple files at once. This behavior originates from the underlying Easy Markdown Editor and its imageUploadFunction, which handles only one file at a time. The relevant implementation is here:
    EasyMDE source.

    On the Blazorise side, event handling follows a similar pattern to FileEdit, which maintains the same event arguments (Files instead of File). That said, I think we should improve this, at least by documenting it. Additionally, I believe I can "group" the individual calls from js into a single call to .NET.

Unreachable Files

The current fixed implementation follows one flow—it keeps files in memory "forever". This is by design because Blazorise cannot effectively predict when a user will call WriteToStream.

This issue could be mitigated by deleting the blob from the dictionary (where the files persist "forever") once it has been written to a stream. However, doing so would prevent the user from writing to the stream again or later:

private async Task ImageUploadChanged(FileChangedEventArgs events)
{ 
    fileForLater = events.Files.First();
}

IFileEntry? fileForLater;

async Task SomeMethodThatIsCalledLater()
{
    if (fileForLater is null) return; 
    // C# file reference is not null, but the actual blob state in JS is unknown 
    using var memoryStream = new MemoryStream();
    await fileForLater.WriteToStreamAsync(memoryStream);
}

The implementation problem lies in the fact that the C# reference is not synchronized with its content on the js side, and there is currently no way to verify this from C# code.

(also) for this reason, the documentation includes a try-catch block around this logic. However, I think it would be better to streamline the solution with something like TryToWriteToStreamAsync...

Is it worth improving that @stsrki ?

@tesar-tech tesar-tech requested a review from stsrki February 19, 2025 09:39
@stsrki
Copy link
Collaborator

stsrki commented Feb 19, 2025

The fix seems to be working fine. That's good.

Regarding the suggested changes. If you think you could improve it and make it without introducing any new APIs while still making it sync between C# and JS reference, then you're free to go. I will open new ticket for that.

Anyways, before merging, please update the docs and mention the current behavior and the reasoning behind it so that people are aware of it.

@stsrki stsrki merged commit 6549e9a into rel-1.7 Feb 19, 2025
2 checks passed
@stsrki stsrki deleted the sup/rel-1.7/Markdown-ImageUploadChanged-throws-exception-on-multiple-files branch February 19, 2025 11:09
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants