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

External assets #3220

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

External assets #3220

wants to merge 21 commits into from

Conversation

abulvenz
Copy link
Contributor

@abulvenz abulvenz commented May 3, 2024

When referencing external modules, the files for the asset folder should be somehow shippable. This is a proposal for a simple helper function that copies the files to the assets folder of the app that is currently compiled.

# library_module.index.py

# In the library module reference the 
# asset files with the relative file path to the including script.
@rx.page("/")
def index() -> rx.Component:
    return rx.center(
        rx.image(src=rx.asset("test.png",dir="subfolder"))
    )

During compilation those files are copied to assets/external/library_module/index/subfolder/test.png.

In case you want to make assets from your library includable for calling modules, you can expose them like this:

class Assets:
    test_png = rx.asset("test.png", "hang_it_up")
    script_js = rx.asset("my_component_logic.js")

@benedikt-bartscher
Copy link
Contributor

benedikt-bartscher commented May 4, 2024

@abulvenz how about symlinking instead of copying?
Edit: Not sure how symlinks behave on Windows

reflex/app.py Outdated
dir = caller_module_path if dir == "." else caller_module_path + "/" + dir
assets = constants.Dirs.APP_ASSETS
external = constants.Dirs.EXTERNAL_APP_ASSETS
Path(Path(cwd) / assets / external / dir).mkdir(parents=True, exist_ok=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Create a variable for Path(Path(cwd) / assets / external / dir) first and use it in the next lines. Also check if the source exists with pathlib's .resolve() method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Lendemor I hope you are fine with using symlinks here, since we are not linking directories. That seems to be the only difference. https://docs.python.org/3/library/pathlib.html#pathlib.Path.symlink_to

@benedikt-bartscher I just tested what happens when src_file is not present. It already complains No such file or directory. OK, or wrap it into a an AssetNotPresent?

Copy link
Contributor

Choose a reason for hiding this comment

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

@abulvenz awesome, if there is already a FileNotFound exception, I think we are fine, no additional wrapping needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

@abulvenz Sorry for delay in review, was on break for a few days.

It's passing windows CI so it should be fine I think.

I'm not sure about adding it inside app.py however, so could you move it to it's own file? Maybe inside reflex/experimental so we'll get some time to test it in real situations before making it official.

We'll also need to add unit tests for it.

Copy link
Contributor Author

@abulvenz abulvenz May 15, 2024

Choose a reason for hiding this comment

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

Thank you @Lendemor, After v0.5.0 everybody earned a small break 🚀 . I will move it to experimental and try to add a unit test.

@Lendemor
Copy link
Collaborator

Lendemor commented May 5, 2024

@abulvenz how about symlinking instead of copying? Edit: Not sure how symlinks behave on Windows

Symlink doesn't work on Windows, we had this in the beginning for syncing between assets and .web/public folder, but had to remove it for Windows.

@abulvenz
Copy link
Contributor Author

abulvenz commented May 7, 2024

Symlink doesn't work on Windows, we had this in the beginning for syncing between assets and .web/public folder, but had to remove it for Windows.

@Lendemor Should I change it back? 🧑‍🔧

Is anything else missing here?

@abulvenz
Copy link
Contributor Author

@Lendemor Is it possible that I cancelled your approval by merging main? Sorry. 🥣

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

3 participants