Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add FileDownloader service for Blazor apps #43076
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
Add FileDownloader service for Blazor apps #43076
Changes from all commits
4be17c5
35fcfb9
4f09b1f
214de09
42fad85
50d1f7b
6a8884e
92f4b87
dd191dd
a8fcbae
cd96575
c0198d8
7233f2f
39a1379
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TanayParikh, could you comment on the efficiency of sending streams versus byte arrays, both for Server and WebAssembly? In my memory, on WebAssembly at least, sending a
byte[]
is super efficient because it's done in a single step without even copying the data in memory.If that is the case, it would be advantageous to have two different internal APIs, e.g.,
Blazor._internal.downloadFileArray
andBlazor._internal.downloadFileStream
, and pick between them based on the supplied data (so we don't have to normalize both cases to streams). For Server I'm not sure whether we want to preferbyte[]
orStream
if we're given abyte[]
.However if I'm misremembering and there isn't a perf advantage to using
byte[]
on WebAssembly then this looks good as-is!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct,
byte[]
interop is achieved via anInvokeUnmarshalled
call in WebAssembly, making it super efficient.aspnetcore/src/Components/WebAssembly/JSInterop/src/WebAssemblyJSRuntime.cs
Line 76 in bc3a2dd
aspnetcore/src/Components/Web.JS/src/Boot.WebAssembly.ts
Line 203 in bc3a2dd
Streaming interop just leverages this unmarshalled interop for the transfer of the underlying byte chunks:
aspnetcore/src/Components/Shared/src/TransmitDataStreamToJS.cs
Line 24 in bc3a2dd
For WebAssembly
byte[]
s can be handled separately so we can take advantage of this feature. The part which is a bit unclear is whether this performance benefit is worth having the additional complexity specifically for this case.For Blazor Server, given the SignalR message size limitation (and assuming downloads are likely to be greater than 32kb), I don't think we're going to see too much of a benefit having a separate path for
byte[]
downloads.Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great that you've implemented
downloadFile
usingBlob
andcreateObjectURL
since that's the only way that works on all browsers. We do need this.However, this approach does have the drawback that it involves:
[1] Buffering the entire file in browser memory, even if it's gigabytes
[2] Not showing the "save as" dialog until we've got the while file data into browser memory
On Chromium browsers (and probably other browsers in the future), there's a new spec called the FileSystem API that provides a better option, whereby we can show the "save as" dialog first, and then stream the data into the file without buffering it all in memory. It works like this:
You can detect whether or not the FileSystem API is available in the current browser by checking if
typeof(window.showSaveFilePicker) === 'function'
.So, in order to provide a better experience on Chromium (and future) browsers, do you think we could detect and
window.showSaveFilePicker
and switch between the two implementations based on that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I'm aware this means the user won't see the file showing up in their "Downloads" UI within the browser. But if the Blob approach doesn't really solve that either because the download doesn't happen as far as the browser is concerned until the JS code has actually downloaded all the data and buffered it in memory, so you don't see any progress during the file transfer - it's like nothing is happening. So I still think the UX is better with the FileSystem API implementation overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, this would be nice to have.
Do we think we need some sort of way to have progress reporting? I can think that we could do this directly on the .NET side. (Technically users could do this themselves) by wrapping the Stream and firing up an event as it is being read. So, a component could register for that event and update the UI accordingly (Imagine a toast/progress bar type of thing).
That would to a degree for both cases (new API + old API). That said, if we can optionally leverage the new API, that would be great (and orthogonal to my suggestion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is the basis for having a component for this in the future, imagine something like
<DownloadableFile Source="..." />
that displays a button to start the download, and switches to render a progress bar in place as the download is happening. (not saying we have to do it, just throwing the idea out there)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that people can already do this themselves on the .NET side makes it fine to not bake in any secondary feature around progress status right now. Let's not expand the scope of the feature too much.
Agreed that would be cool, but also agreed that it doesn't have to be baked-in as a framework feature when people can do it themselves. Maybe in the long term if there's clear enough customer demand though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Steve, wasn't aware of that API. Took a brief look and another couple of things to note for
showSaveFilePicker
:caniuse
has this at 27% of all users and 75% of desktop users. Anecdotally, given users are probably more likely to be downloading on a Desktop than a mobile device, I still think this would be worthwhile.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Do you know if Selenium gives us any sensible way to automate a test about file downloads? I'm unsure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could hijack the JS on the Blazor instance to skip the part about actually showing the dialog (I do not believe there is a way to deal with it in Selenium)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds a bit complicated. We could add it to CTI tests I guess, even though it's a bit unusual to test a relatively low-level feature via CTI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this doesn't look like something that's encouraged:
https://www.selenium.dev/documentation/test_practices/discouraged/file_downloads/
There are workarounds, but I don't think it's worth the additional long term maintenance complexity. We'll have to defer this to CTI as you mention.