Skip to content

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

Closed

Conversation

cerasamson
Copy link
Contributor

This adds a globally registered FileDownloader service which allows users to download a file from a Blazor app. The download is triggered on an event, such as a <button> click. The file data (Stream or byte[]) is read in .NET and the JS method downloadFile is invoked to stream file contents to the client.

Example API usage: await FileDownload.SaveAs(fileName, data);

Example in Blazor Server App:
image

Fixes #25274

@cerasamson cerasamson added the area-blazor Includes: Blazor, Razor Components label Aug 3, 2022
anchorElement.href = url;
anchorElement.download = fileName;
anchorElement.click();
anchorElement.remove();
Copy link
Contributor

@campersau campersau Aug 3, 2022

Choose a reason for hiding this comment

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

I think the anchorElement needs to be added to the DOM before the click() e.g.

document.body.appendChild(anchorElement);

to make it work in Firefox. At least that is what I have in my own custom implementation.
Otherwise the removal isn't needed I guess.

Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

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

Looking great so far! Some comments below.

Note; we'll also have to add support for the WebView use case (should likely just be a matter of ensuring the IFileDownloader is added to the WebView DI container) (and validating using the Photino test app in the repo).


using var streamRef = new DotNetStreamReference(stream: data);

await _jsRuntime.InvokeVoidAsync("Blazor._internal.downloadFile", streamRef, fileName);
Copy link
Member

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 and Blazor._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 prefer byte[] or Stream if we're given a byte[].

However if I'm misremembering and there isn't a perf advantage to using byte[] on WebAssembly then this looks good as-is!

Copy link
Contributor

@TanayParikh TanayParikh Aug 4, 2022

Choose a reason for hiding this comment

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

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.

That's correct, byte[] interop is achieved via an InvokeUnmarshalled call in WebAssembly, making it super efficient.

InvokeUnmarshalled<int, byte[], object>("Blazor._internal.receiveByteArray", id, data);

const dataByteArray = monoPlatform.toUint8Array(data);

Streaming interop just leverages this unmarshalled interop for the transfer of the underlying byte chunks:

await runtime.InvokeVoidAsync("Blazor._internal.receiveDotNetDataStream", streamId, buffer, bytesRead, null);

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.

anchorElement.click();
anchorElement.remove();
URL.revokeObjectURL(url);
}
Copy link
Member

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 using Blob and createObjectURL 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:

// This is actually supplied by JS interop - no need to construct a ReadableStream in the new code
const readableStream = new Response(new TextEncoder().encode('Some contents here')).body;

// Show the "save as dialog"
let fileWriter;
try {
    const fileHandle = await window.showSaveFilePicker();
    fileWriter = await fileHandle.createWritable();
} catch {
    // User pressed cancel, so abort the whole thing
    return;
}

const reader = readableStream.getReader();
while (true) {
    const readResult = await reader.read();
    if (readResult.done) {
        break;
    }

    await fileWriter.write(readResult.value);
}

await fileWriter.close();

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?

Copy link
Member

@SteveSandersonMS SteveSandersonMS Aug 4, 2022

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.

Copy link
Member

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)

Copy link
Member

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)

Copy link
Member

Choose a reason for hiding this comment

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

Do we think we need some sort of way to have progress reporting? ... Technically users could do this themselves

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.

imagine something like 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

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!

Copy link
Contributor

@TanayParikh TanayParikh Aug 4, 2022

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:

  • Chromium desktop exclusive (doesn't work on mobile)
  • Requires secure context

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.

var data = GetFileStream();
await FileDownload.SaveAs(fileName, data);
}
}
Copy link
Member

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.

Copy link
Member

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)

Copy link
Member

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.

Copy link
Contributor

@TanayParikh TanayParikh Aug 4, 2022

Choose a reason for hiding this comment

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

Do you know if Selenium gives us any sensible way to automate a test about file downloads?

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.

@SteveSandersonMS
Copy link
Member

This is looking excellent!

return;
}

const reader = data.getReader();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm is this working as-is? I was under the impression data is a DotNetStream here, hence we may have to do:

Suggested change
const reader = data.getReader();
const reader = await data.stream().getReader();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This initially caused an unhandled exception. I believe data had to be converted into a ReadableStream in order for the getReader() method to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, is data a DotNetStream here? If so, we can get the readable stream directly by calling stream:

/**
* Supplies a readable stream of data being sent from .NET.
*/
stream(): Promise<ReadableStream> {
return this._streamPromise;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This initially caused an unhandled exception.

What exception were you running into?

Copy link
Contributor Author

@cerasamson cerasamson Aug 10, 2022

Choose a reason for hiding this comment

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

When testing this with FileDownloaderComponent.razor, data is a byte[] being converted to a MemoryStream instead of a DotNetStreamReference.

As a result, I was getting the following exception: TypeError: e.stream(...).getReader is not a function

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@cerasamson cerasamson Aug 10, 2022

Choose a reason for hiding this comment

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

The second option is working and data as a DotNetStreamReference is being saved to the file correctly.

The exception for dataStream.getReader() is being handled, but no data is populating the file. I will look into this.

@TanayParikh TanayParikh removed this from the 7.0-rc1 milestone Aug 15, 2022
@mkArtakMSFT
Copy link
Contributor

Closing as this went stale for some time now.

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
Development

Successfully merging this pull request may close these issues.

Blazor Image component to display images that are not accessible through HTTP endpoints
6 participants