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

[WIP] Initial implementation of asset serving in DashR #64

Merged
merged 28 commits into from
Apr 9, 2019
Merged

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Mar 21, 2019

This PR proposes to provide support for locally serving assets from a specified assets folder. As in Dash for Python, the following features would be implemented upon merging these commits (/assets below refers to whichever folder is specified in assets_folder):

  • Assets folder auto resolution (/assets) at the root of the project by default.
  • Assets folder may be passed as parameter, customizing the path as desired
  • JS/CSS files in /assets are served in the index
  • An assets_ignore filter permits excluding assets matching specified regex
  • Assets are cached by the browser, and uncached when modified
  • All files in /assets are served statically
  • The generated assets URL respects routes/requests pathname prefix
  • Easy retrieval of assets URLs is possible: get_asset_url(asset) returns a ready-to-insert URL string
  • A favicon.ico is served as the title icon of the page, if in the root directory of /assets

There are is at least two one items that have has not yet been implemented, but which are is desirable:

  • A favicon.ico is not currently served as the title icon of the page
  • Automatic compression of served assets is not currently supported, but should be feasible

While the above features have been tested, formal tests asserting that these elements are provided should be be written. An example of the Python code which performs this task is available here:

https://github.com/plotly/dash/blob/master/tests/test_resources.py

Closes #46.

@rpkyle rpkyle self-assigned this Mar 21, 2019
R/dash.R Outdated Show resolved Hide resolved
R/dash.R Outdated Show resolved Hide resolved
@rpkyle
Copy link
Contributor Author

rpkyle commented Apr 7, 2019

@alexcjohnson Let me know if this is looking a bit better; I've also added support for a favicon.ico in 74392b1.

if (!(dir.exists(private$assets_folder))) {
warning(sprintf(
"The supplied assets folder, '%s' could not be found in the project directory.",
private$assets_folder),
Copy link
Collaborator

Choose a reason for hiding this comment

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

dashPy doesn't look like it has this check. I like the check, but it'll cause problems for people who start an app with the default assets here but they don't have (or need) any assets.

Simplest solution I see is to ignore this warning if the folder is the default. It would be even better if we could ignore the error if the user didn't provide a folder name - meaning still give the warning if they provided a value that happens to match the default - but I don't think it would be a good tradeoff to drop the default from the function signature and lose that automatic documentation.

Copy link
Contributor Author

@rpkyle rpkyle Apr 8, 2019

Choose a reason for hiding this comment

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

Ah, good point. (I should just have a template for your PR reviews where this is the default response.)

So, ultimately what we want is to keep this warning, but

  • don't display it when assets_folder is set to assets, the default, and the folder cannot be found
  • display the warning when assets_folder is user-supplied, ≠ assets, and cannot be found

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

@rpkyle just one new one, the comment I just made about the missing assets folder warning, then this looks great. Put this PR near the top of your list for testing once there's a framework in place for that, as there's a lot of logic here. But assuming you've tried it out in a couple of scenarios I'm happy to approve it now. Nice work! 💃

@rpkyle rpkyle merged commit 964e06b into master Apr 9, 2019
@rpkyle rpkyle deleted the 0.0.5 branch April 9, 2019 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asset serving
2 participants