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

add download event #1797

Merged
merged 3 commits into from
Sep 13, 2023
Merged

add download event #1797

merged 3 commits into from
Sep 13, 2023

Conversation

Lendemor
Copy link
Collaborator

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Description

Add an API for easily downloading files.

Usage is as follow:

rx.button("Download file", on_click=rx.download("/uri/for/file.extension", "target_file.extension")

or

def export_data(self):
    ...
    # do export logic here and write to filepath
    # then
    yield rx.download(filepath, filename)

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

I'm a little unclear on why this would be preferred over just using a standard link. This seems like more moving parts and harder to deal with error handling.

If I wanted to provide a download link, I would keep the href in a state Var and render a normal <a> tag in the frontend.

@Lendemor
Copy link
Collaborator Author

I'm a little unclear on why this would be preferred over just using a standard link. This seems like more moving parts and harder to deal with error handling.

If I wanted to provide a download link, I would keep the href in a state Var and render a normal <a> tag in the frontend.

This only work if the file is already available beforehand.
Using a <a> with a href doesn't let you execute an event handler before starting the download.

Take an example where you have a data_table and an export button.
You can see your data in the datatable, but the file doesn't exist yet (you could want to apply some filter before export etc..)
Clicking an export button will trigger an event handler where you can both do your logic and then yield the front side download event.

@masenf
Copy link
Collaborator

masenf commented Sep 11, 2023

Using a <a> with a href doesn't let you execute an event handler before starting the download.

Fair enough, but is it possible make the hidden <a> that gets clicked link to the actual URL and let the client browser deal with the error handling? Or is there some security restriction that requires the clicked link href to be a URL.createObjectURL?

@Lendemor Lendemor requested a review from masenf September 12, 2023 17:40
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Code works well for me! Just one suggestion

reflex/event.py Outdated
@@ -330,6 +330,29 @@ def set_clipboard(content: str) -> EventSpec:
)


def download(url: str, filename: str) -> EventSpec:
Copy link
Contributor

Choose a reason for hiding this comment

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

If they don't provide a filename, maybe we can default to the last part of the path in the url? Basically give it the same filename it was saved as

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure we could do that

@Lendemor Lendemor requested a review from picklelo September 13, 2023 15:04
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

awesome, great work

@@ -330,6 +330,34 @@ def set_clipboard(content: str) -> EventSpec:
)


def download(url: str, filename: Optional[str] = None) -> EventSpec:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Instead of Optional we can do str | None now

@picklelo picklelo merged commit b378827 into main Sep 13, 2023
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

Sorry didn't re-review, but the merged implementation is killer, thanks for making the changes @Lendemor

@picklelo picklelo deleted the lendemor/add_download_event branch September 16, 2023 00:27
Alek99 pushed a commit that referenced this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants