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

fix download for uploaded files #3041

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Lendemor
Copy link
Collaborator

@Lendemor Lendemor commented Apr 8, 2024

No description provided.

@masenf
Copy link
Collaborator

masenf commented Apr 8, 2024

I think the existing code already mostly works without adding all this extra structure. There just seems to be a problem when inlining the rx.download event handler:

import reflex as rx

class State(rx.State):
    button_text: str = "Download"
    def do_download(self):
        filename = "foo.txt"
        (rx.get_upload_dir() / filename).write_text("test")
        return rx.download(rx.get_upload_url(filename))


@rx.page()
def index():
    return rx.vstack(
        # Works
        rx.button(State.button_text, on_click=State.do_download),
        # Works
        rx.link("Download", href=rx.get_upload_url("foo.txt")),
        # This doesn't seem to work...
        # rx.button(State.button_text, on_click=rx.download(rx.get_upload_url("foo.txt"))),
    )

app = rx.App(overlay_component=None)

@masenf
Copy link
Collaborator

masenf commented Apr 8, 2024

diff --git a/reflex/components/core/upload.py b/reflex/components/core/upload.py
index dfc62bc0..ae1e5f29 100644
--- a/reflex/components/core/upload.py
+++ b/reflex/components/core/upload.py
@@ -140,7 +140,7 @@ def get_upload_url(file_path: str) -> Var[str]:
     """
     Upload.is_used = True
 
-    return Var.create_safe(f"{uploaded_files_url_prefix}/{file_path}")
+    return Var.create_safe(f"{uploaded_files_url_prefix}/{file_path}", _var_is_string=True)
 
 
 def _on_drop_spec(files: Var):

This seems sufficient to get things working as expected without introducing new API

masenf
masenf previously approved these changes Apr 8, 2024
getBackendURL(env.UPLOAD)
);
if (event.payload.filename) {
a.href = a.href + "?filename=" + event.payload.filename;
Copy link
Collaborator

Choose a reason for hiding this comment

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

potentially minor issue, but is there a case where the href might already have query params?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so.

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.

actually, i think this regressed the rx.download from backend case, that's no longer working in my sample code that i posted above

@masenf
Copy link
Collaborator

masenf commented Apr 8, 2024

Here is the repro

import reflex as rx

class State(rx.State):
    button_text: str = "Download"
    def do_download(self):
        filename = "foo.txt"
        (rx.get_upload_dir() / filename).write_text("test")
        print("returning download event")
        #return rx.download(rx.get_upload_url(filename), filename="bar.csv")
        return rx.download(rx.get_upload_url(filename))

    def do_download_assets(self):
        return rx.download("/favicon.ico")

    def something_else(self):
        return rx.console_log("something else")


@rx.page()
def index():
    return rx.vstack(
        rx.button(State.button_text, on_click=State.do_download),
        rx.button("Download favicon.ico", on_click=State.do_download_assets),
        rx.button("Something else", on_click=State.something_else),
        rx.button("Download2", on_click=rx.download(data="test", filename="foo.csv")),
        rx.link("Download", href=rx.get_upload_url("foo.txt"), download="foo.csv"),
    )

app = rx.App(overlay_component=None)

If you click the top "Download" button, it works once, then the "Something Else" and "Download favicon.ico" buttons stop working until the page is refreshed. Interestingly, the "Download2" button which directly adds the _download event will still work after clicking the top "Download" button. It seems to only affect events that need to go to the backend first.

You can click "Download favicon.ico" or "Something Else" as many times as you want and they seem to keep working, as long as "Download" isn't clicked.

This is really weird and mysterious.

@Lendemor Lendemor marked this pull request as draft April 10, 2024 10:39
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.

None yet

2 participants