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

Support returning asset URLs via public method within Dash class #160

Merged
merged 5 commits into from
Jan 3, 2020

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Dec 27, 2019

The previous implementation of get_asset_url in Dash for R was not particularly useful -- it required passing an object whose asset paths were set on the names attribute of the object, which is used internally by Dash, but not generally available to application developers. It was also unexported, and not used elsewhere in the package -- effectively dead code.

This revision makes the get_asset_url a public method of the Dash class itself, so it is available to developers for use.

If this PR is merged, the method will permit returning the URL of any file in the assets directory, provided that a valid relative path within an app's assets_folder is passed. If the asset is not found, an error is returned.

@rpkyle rpkyle added enhancement parity Modifications to improve parity across Dash implementations dash-stage-in_review dash-meta-prioritized feature request Community request for a package enhancement labels Dec 27, 2019
@rpkyle rpkyle added this to the Dash Q1/2020 milestone Dec 27, 2019
@rpkyle rpkyle self-assigned this Dec 27, 2019
R/dash.R Outdated
# asset_path should be prepended with the full app root & assets path
# if leading slash(es) present in asset_path, remove them before
# assembling full asset path
asset_path <- file.path(dirname(private$app_root_path),
Copy link
Contributor

Choose a reason for hiding this comment

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

I encountered the following error when running this line to generate the asset_path:

Error in dirname(private$app_root_path) : a character vector argument expected

Digging a bit deeper into it, the app_root_path private field in Dash calls the function getAppPath from utils.R upon initialization of the Dash class, and returns the value FALSE if the app isn't run via source().

A potential fix could be:
ifelse(is.logical(private$app_root_path), "", dirname(private$app_root_path)), but I'm not exactly sure what needs to be there when the condition is TRUE to get the correct path.

A more permanent fix would be to tweak the getAppPath function to deliver a more comprehensive return value for the else condition.

Copy link
Contributor

@HammadTheOne HammadTheOne left a comment

Choose a reason for hiding this comment

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

Hey Ryan, I didn't see any real issues with the code, it's written concisely and makes sense, the comments make it easy to follow.

The get_asset_url method works perfectly fine when running the app from source('app.R'), but has an issue when run from the script. Once that's figured out, it should be good to go.

@rpkyle
Copy link
Contributor Author

rpkyle commented Dec 28, 2019

Hey Ryan, I didn't see any real issues with the code, it's written concisely and makes sense, the comments make it easy to follow.

The get_asset_url method works perfectly fine when running the app from source('app.R'), but has an issue when run from the script. Once that's figured out, it should be good to go.

Hmm, yes. This is going to be tricky to resolve. We don't have a reliable way to determine the application root unless code is executed using source(). Basically, source() calls parse(), which populates srcref attributes, as described here:

https://journal.r-project.org/archive/2010-2/RJournal_2010-2_Murdoch.pdf

The srcref attributes are very handy, since they are a relatively safe way of identifying the location of R code.

Otherwise, we're basically making our best guess at the value of app_root_path whenever code is pasted/entered into the console.

@alexcjohnson Any suggestions? We're trying to reproduce the behaviour of get_asset_url in R to mimic that in Python, but running into a snag when code is not loaded via source().

@alexcjohnson
Copy link
Collaborator

Otherwise, we're basically making our best guess at the value of app_root_path whenever code is pasted/entered into the console.

This concern isn't new in this PR, right? So we can make an issue for it but I wouldn't try to address it in this PR.

When you paste code in the console there's really no reason to expect it comes from anywhere in the filesystem. So it seems to me in fact a bad idea to try to impute some location to it. If users want to work this way, the root path would need to be provided explicitly in the code; otherwise we should steer folks to a workflow in which the app is invoked explicitly via its path.

@HammadTheOne HammadTheOne self-requested a review January 2, 2020 23:38
Copy link
Contributor

@HammadTheOne HammadTheOne left a comment

Choose a reason for hiding this comment

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

Based on the conversation above, it doesn't seem like that particular use-case needs to be an issue tackled with this PR, it might be helpful to take a look at for debugging and testing purposes in the future, but following the usual workflow of a deployed Dash application, this shouldn't come up. Apart from that, the functionality works as intended and I was able to follow along with the same usage as with the Python implementation.

💃

@rpkyle rpkyle merged commit 61e6dfa into dev Jan 3, 2020
@rpkyle rpkyle deleted the 158-get-asset-urls branch January 3, 2020 17:34
rpkyle added a commit that referenced this pull request Jan 4, 2020
* Provide support for no_update in Dash for R (#111)

* Use dev_tools_prune_errors instead of pruned_errors (#113)

* Better handling for user-defined error conditions in debug mode (#116)

* Provide support for multiple outputs (#119)

* Provide support for hot reloading in Dash for R (#127)

* Implement support for clientside callbacks in Dash for R (#130)

* Add line number context to stack traces when srcrefs are available (#133)

* Update dash-renderer to 1.2.2 and fix dev tools UI display of stack traces (#137)

* Support for meta tags in Dash for R (#142)

* Fixes for hot reloading interval handling and refreshing apps within viewer pane (#148)

* Support for asynchronous loading/compression in Dash for R (#157)

* Support returning asset URLs via public method within Dash class (#160)

* Minor fix for get_asset_url + docs, add url_base_pathname (#161)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dash-meta-prioritized enhancement feature request Community request for a package enhancement parity Modifications to improve parity across Dash implementations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants