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

Don't assume all scripts are given as character variables. #321

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@

* `copyDependencyToDir()` no longer creates empty directories for dependencies that do not have any files. (@gadenbuie, #276)

* Closed #320: `copyDependencyToDir()` now works with dependencies
with specified attributes. (@dmurdoch, #321)

# htmltools 0.5.2

## Breaking Changes
Expand Down
14 changes: 9 additions & 5 deletions R/html_dependency.R
Original file line number Diff line number Diff line change
Expand Up @@ -354,9 +354,12 @@ copyDependencyToDir <- function(dependency, outputDir, mustWork = TRUE) {
dir.create(target_dir)
dependency$src$file <- normalizePath(target_dir, "/", TRUE)

files <- if (dependency$all_files) list.files(dir) else {
unlist(dependency[c('script', 'stylesheet', 'attachment')])
}
if (dependency$all_files)
files <- list.files(dir)
else
files <- c(pluck_src(dependency$script),
pluck_src(dependency$stylesheet),
pluck_src(dependency$attachment))

if (length(files) == 0) {
# This dependency doesn't include any files
Expand All @@ -366,11 +369,12 @@ copyDependencyToDir <- function(dependency, outputDir, mustWork = TRUE) {
}

srcfiles <- file.path(dir, files)
if (any(!file.exists(srcfiles))) {
missing_srcfiles <- !file.exists(srcfiles)
if (any(missing_srcfiles)) {
stop(
sprintf(
"Can't copy dependency files that don't exist: '%s'",
paste(srcfiles, collapse = "', '")
paste(srcfiles[missing_srcfiles], collapse = "', '")
)
)
}
Expand Down
9 changes: 9 additions & 0 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,12 @@ anyUnnamed <- function(x) {
# List with name attribute; check for any ""
any(!nzchar(nms))
}

# Get the source filename out of a script or similar
# entry.
pluck_src <- function(x) {
if (is.character(x)) return(x)
if (is.list(x) && "src" %in% names(x)) return(x$src)
if (is.list(x)) return(vapply(x, pluck_src, ""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm finding it a bit challenging to understand what exactly this function is trying to do. I think it's in part because the shape of the data going in is pretty loose -- that's something that has long been a cause of issues when dealing with html dependency objects.

In this particular case, I'm wondering about the NULL return and the vapply(). If this vapply() is called on the object and one of the items returns NULL, that will result in an error. Will it ever get to the NULL at the end of the function? If not, it would make sense to have it throw an error with a helpful message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just remembered that we actually have a spec for the HTML dependency data structure, in Shiny's TypeScript code. We can use this to reason about how this code should work.
https://github.com/rstudio/shiny/blob/474f1400/srcts/src/shiny/render.ts#L79-L128

NULL
}
87 changes: 87 additions & 0 deletions tests/testthat/test-deps.r
Original file line number Diff line number Diff line change
Expand Up @@ -357,3 +357,90 @@ test_that("copyDependencyToDir() doesn't create an empty directory", {
# to keep relativeTo() from throwing an error later in Rmd render process
expect_match(copied_dep$src$file, normalizePath(tmp_rmd, "/", TRUE), fixed = TRUE)
})

test_that("copyDependencyToDir() handles attributes", {
tmp_dep <- tempfile("dep")
dir.create(tmp_dep)
on.exit(unlink(tmp_dep))

tmp_txt <- "temp.txt"
path <- file.path(tmp_dep, tmp_txt)
writeLines("Some text in the text/plain dep", path)

tmp_js <- "javascript.js"
path <- file.path(tmp_dep, tmp_js)
writeLines('console.log("log message");', path)

tmp_rmd <- tempfile("rmd_files")
dir.create(tmp_rmd)
on.exit(unlink(tmp_rmd), add = TRUE)

dep_with_attributes <- htmltools::htmlDependency(
name = "textPlain",
version = "9.9.9",
src = tmp_dep,
script = list(src = tmp_txt, type = "text/plain"),
all_files = FALSE
)

dep_without <- htmltools::htmlDependency(
name = "textPlain",
version = "9.9.9",
src = tmp_dep,
script = tmp_js,
all_files = FALSE
)

dep_with_both <- htmltools::htmlDependency(
name = "textPlain",
version = "9.9.9",
src = tmp_dep,
script = list(tmp_js,
list(src = tmp_txt, type = "text/plain")),
all_files = FALSE
)

dep_with_one_nested <- htmltools::htmlDependency(
name = "textPlain",
version = "9.9.9",
src = tmp_dep,
script = list(list(src = tmp_txt, type = "text/plain")),
all_files = FALSE
)

dep_with_missings <- htmltools::htmlDependency(
name = "textPlain",
version = "9.9.9",
src = tmp_dep,
script = list(tmp_js,
"foobar1",
list(src = "foobar2")),
all_files = FALSE
)

# None of these except the last should trigger errors as
# the first two did in issue #320

copyDependencyToDir(dep_with_attributes, tmp_rmd)
expect_equal(dir(tmp_rmd, recursive = TRUE),
"textPlain-9.9.9/temp.txt")

unlink(dir(tmp_rmd, recursive = TRUE))
copyDependencyToDir(dep_with_both, tmp_rmd)
expect_equal(dir(tmp_rmd, recursive = TRUE),
c("textPlain-9.9.9/javascript.js",
"textPlain-9.9.9/temp.txt"))

unlink(dir(tmp_rmd, recursive = TRUE))
copyDependencyToDir(dep_without, tmp_rmd)
expect_equal(dir(tmp_rmd, recursive = TRUE),
"textPlain-9.9.9/javascript.js")

unlink(dir(tmp_rmd, recursive = TRUE))
copyDependencyToDir(dep_with_one_nested, tmp_rmd)
expect_equal(dir(tmp_rmd, recursive = TRUE),
"textPlain-9.9.9/temp.txt")

expect_error(copyDependencyToDir(dep_with_missings, tmp_rmd))

})