-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
External assets #3220
Conversation
@abulvenz how about symlinking instead of copying? |
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Symlink doesn't work on Windows, we had this in the beginning for syncing between |
@Lendemor Should I change it back? 🧑🔧 Is anything else missing here? |
@Lendemor Is it possible that I cancelled your approval by merging main? Sorry. 🥣 |
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.
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: