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

[LiveComponent] Add support for downloading files from LiveActions (Experimental) #2483

Open
wants to merge 12 commits into
base: 2.x
Choose a base branch
from

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Jan 5, 2025

Q A
Bug fix? no
New feature? yes
Issues Close #1516
License MIT

This pull request introduces a new experimental feature for handling file downloads in LiveActions.

Caution

As stated here, this feature -if merged- will keep an experimental status, for everyone to test and see how it goes. We cannot ensure BC promise on features without testing it IRL first.

TL;DR;

Works perfectly with existing files from the Filesystem.

 #[LiveAction]
public function download(): BinaryFileResponse
{
    $file = $this->projectDir . '/docs/legals/terms.pdf');

    return new LiveDownloadResponse($file);
}

Works also with file generated at runtime.

 #[LiveAction]
public function generate(): BinaryFileResponse
{
    $file = $this->projectDir . '/docs/legals/terms.pdf');
    
    $file new SplTempFileObject($file);

    return new LiveDownloadResponse($file);
}

Also

I started the demo, just need to complete some texts + add a bit of style

Scope of this

This PR taks a very narrow scope on this thing, i already started working on cool things for upload (as I also needed to for work..)

Let's stay focus on this here (discussions / issues open for any new idea / MR)

@carsonbot carsonbot added Feature New Feature LiveComponent Status: Needs Review Needs to be reviewed labels Jan 5, 2025
@smnandre smnandre requested review from kbond, Kocal and WebMamba January 5, 2025 02:48
Copy link

github-actions bot commented Jan 5, 2025

📊 Packages dist files size difference

Thanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
Please review the changes and make sure they are expected.

FileBefore (Size / Gzip)After (Size / Gzip)
LiveComponent
Backend/BackendResponse.d.ts 136 B / 139 B 166 B+22% 📈 / 148 B+6% 📈
live_controller.js 119.92 kB / 23.37 kB 120.96 kB+1% 📈 / 23.6 kB+1% 📈

Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

This solution is so simple 🤩

src/LiveComponent/assets/src/Backend/BackendResponse.ts Outdated Show resolved Hide resolved
throw new Error('Invalid LiveDownload response');
}

const fileSize = Number.parseInt(headers.get('Content-Length') || '0');
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 really want to assume that if Content-Length header is absent, then it is equal to 0?
In which case the header can be absent? (I didn't see the rest of the PR yet!)

Copy link
Member Author

Choose a reason for hiding this comment

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

In which case the header can be absent?

It should not be. This method is not made to stream movies, but files generated using the liveprops values.

Elsewhen, developers should send an URL to the JS and then open some stream, or redirect to another page, etc...

that if Content-Length header is absent, then it is equal to 0

No, absolutely not. But if it is unknown, we need to compute it on the fly then. Not implemented yet, but linked to the other questions below :)

Comment on lines 322 to 324
if (fileSize > 10000000) {
throw new Error('File is too large to download (10MB limit)');
}
Copy link
Member

Choose a reason for hiding this comment

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

More correct and easier to understand:

Suggested change
if (fileSize > 10000000) {
throw new Error('File is too large to download (10MB limit)');
}
if (fileSize > 10 * 1024 * 1024) {
throw new Error('File is too large to download (10MB limit)');
}

Also, why do you want to limit the file size? It can be a nice feature, but shouldn't it be configurable by the developer?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the part we need to discuss. Don't forget the user has NOT accepted this download in any way for now.

Also, you cannot store 1GB in the DOM before saving the file.

We have options after:

  • streaming toward the FileStorage api (where user would then decide where and how to save the file)
  • allowing this only on button with attribute "download" (my preference for now)

src/LiveComponent/assets/src/Component/index.ts Outdated Show resolved Hide resolved
src/LiveComponent/assets/src/Component/index.ts Outdated Show resolved Hide resolved
Comment on lines 262 to 264
if (!$event->getControllerResult()->headers->has(self::DOWNLOAD_HEADER)) {

}
Copy link
Member

Choose a reason for hiding this comment

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

Dead code

Comment on lines 23 to 25
if ((!$file instanceof SplFileInfo)) {
throw new \InvalidArgumentException(sprintf('The file "%s" does not exist.', $file));
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure to understand when this code can be executed, $file can either be a string or SplFileInfo, and when it's a string, we re-assign $file to a SplFileInfo.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the condition is wrong?

parent::__construct($file, 200, [
self::HEADER_LIVE_DOWNLOAD => 1,
'Content-Disposition' => HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, $filename ?? basename($file)),
'Content-Type' => 'application/octet-stream',
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use the $file's mime type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hesitated (keep dep light) and chose not to...

But in fact it's a good idea and Mime is a very small component.

So let's make it a requirement for LiveComponent ? Or only when using downloads / uploads ?

wdyt ?

Copy link
Member

@Kocal Kocal Jan 5, 2025

Choose a reason for hiding this comment

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

I suggest to put it in suggest composer.json section (I don't remember what is the rule in Symfony, cc @nicolas-grekas), and then runtime check if symfony/mime exists (when using downloads/uploads)

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure there is no suggest in Symfony composer packages..

We have other situations in UX where we do not require a package (i.e. in Autocomplete for Form ..)

My question was more: should we require it for everyone, or keep it as an "runtime" dependency ?

Copy link
Member

Choose a reason for hiding this comment

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

runtime dependency

Copy link
Member

Choose a reason for hiding this comment

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

Why aren't you deferring to BinaryFileResponse for this logic?

Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for the header?

In the frontend, if not a standard/known content type, but has a content disposition (and an other requirements), trigger the download? This way we could say "to enable file downloads, return a response that has a content disposition"

src/LiveComponent/src/LiveDownloadResponse.php Outdated Show resolved Hide resolved
@norkunas
Copy link
Contributor

norkunas commented Jan 6, 2025

What about streams from flysystem? We'll need to always load files into memory?

@smnandre
Copy link
Member Author

smnandre commented Jan 7, 2025

What about streams from flysystem? We'll need to always load files into memory?

With that method, i fear yes. That's why the limit. And browsers can change their policy soon to prevent abuse.

We also can ask for a Filesystem API grant.

Again, will need to test one way or the other, locally i can stream a full 8GB file in one second so ... :|

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

This is awesome! I'll let someone else review the frontend code.

For the back end:

  1. I wish we weren't restricted to BinaryFileResponse - I'd like to be able to use with StreamedResponse and a resource.
  2. What are the requirements for a response to trigger as a download on the frontend? Just the special header? Could we add this header in the response event listener (detect if it's a download)?

parent::__construct($file, 200, [
self::HEADER_LIVE_DOWNLOAD => 1,
'Content-Disposition' => HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, $filename ?? basename($file)),
'Content-Type' => 'application/octet-stream',
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't you deferring to BinaryFileResponse for this logic?

@smnandre
Copy link
Member Author

smnandre commented Jan 8, 2025

Why aren't you deferring to BinaryFileResponse for this logic?

How so ? BinaryFile does not add content disposition different than what you pass here as argument.

@smnandre
Copy link
Member Author

smnandre commented Jan 8, 2025

  1. I wish we weren't restricted to BinaryFileResponse - I'd like to be able to use with StreamedResponse and a resource.

I need to continue looking, but the restriction is not here at all, there is just an helper we can provide to offer the best DX possible.

The difficulty is to ensure a ResponseStream will work (without crashing the LiveComponent thread) and this is where problems come from:

  • unknown size is a bad signal
  • content type must be set and be legit

And regarding streaming. I made it work perfectly at home. But i have still not see any use case where downloading a 500MB file via a LiveComponent is a good idea 😅

Anyway we'll need to test IRL a bit to get feedback, issues, etc etc

throw new Error('No filename found in Content-Disposition header');
}

const blob = await backendResponse.getBlob();
Copy link
Member Author

Choose a reason for hiding this comment

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

It depends... if you trigger the "click" and the file start downloading I think browser play the "pipe" role here.

But during my test I managed to crashed pretty violently Chrome and Safari multiple times.

@norkunas
Copy link
Contributor

norkunas commented Jan 9, 2025

And regarding streaming. I made it work perfectly at home. But i have still not see any use case where downloading a 500MB file via a LiveComponent is a good idea 😅

But it's a good idea to reduce resource usage in case memory limit is low.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature LiveComponent Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LiveComponent] Live Actions cannot handle file downloads
5 participants