-
Notifications
You must be signed in to change notification settings - Fork 205
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
Easier specification of external file dependencies #83
base: master
Are you sure you want to change the base?
Changes from 5 commits
5b912ab
94fb57e
fbad626
5f6348b
ed1ba29
cd9478c
04fe1af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
#' Create a file dependency that can be accessed by the client using javascript. | ||
#' @export | ||
fileDependency <- function(filename, version = '0.0.1'){ | ||
htmltools::htmlDependency( | ||
name = basename(tools:::file_path_sans_ext(filename)), | ||
version = version, | ||
src = dirname(filename), | ||
attachment = basename(filename) | ||
) | ||
} | ||
|
||
#' Mark a string as an attachment | ||
#' @export | ||
attachment <- function(x){ | ||
if (!file.exists(x)){ | ||
stop("The attachment ", x, " does not exist") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about just leaving it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion! I will make the change. |
||
} | ||
structure(normalizePath(x), class = unique(c("ATTACHMENT", oldClass(x)))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just used the same code as used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should also probably add a |
||
} | ||
|
||
attachmentDeps <- function(list) { | ||
attachments = rapply(list, function(y){y}, classes = 'ATTACHMENT') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I keep forgetting that this function exists. Thanks for the reminder! |
||
deps = lapply(attachments, fileDependency) | ||
attachments = lapply(as.list(attachments), function(x){ | ||
basename(tools::file_path_sans_ext(x)) | ||
}) | ||
list(attachments = attachments, deps = deps) | ||
} | ||
|
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.
I'm not sure
htmlDependency
is the right substrate to build this on. The reason for this is that htmltools and Shiny assume that once a dependency is satisfied, it's satisfied for good. So in your example, if in a Shiny app, pcs.json changes between one run and the next (and in fact if that's the reason it's changing) then the changes would be ignored.You could fix this by using a random value for the htmlDependency's name, I guess? That would have an unfortunate side effect of never letting go of old values (or at least references to them)... though I think that's still better than not picking up new values.
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.
I wonder... would there be a way to have htmlwidgets instead of resolving the dependency, just insert a link directly into the page?
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.
Good points @jcheng5 . I did not think from this angle. I assumed the external file dependencies to be static resources, and this PR was an attempt to package the boilerplate code into something more automatic.
What you suggest here, would make this mechanism even more powerful, by allowing dynamic external dependencies. Your first comment seems like a simple enough fix that would give us reasonable mileage. However, if there is a way to insert the link directly in the page, I think it would work even better.