diff --git a/NEWS.md b/NEWS.md index c565404a3..e2339d81a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,6 +8,10 @@ * Navsets created with an `id` (e.g. `navset_tab(id = "tabs")`) now use that `id` as their `data-tabsetid`, so their tab panes get stable `tab-tabs-1` style DOM ids instead of ones derived from a random integer. This makes the rendered markup reproducible across renders and easier to target from custom CSS and JavaScript. Navsets without an `id`, and `nav_menu()` dropdowns, keep the random ID. (#1342) +## Bug fixes + +* `bs_dependency_defer()` gains a `cache_key` argument. Set it to a unique value when you create several similar dependencies, so each one stays distinct in the cache. Previously, dependencies built the same way could be mistaken for one another, and every one after the first was served a copy of the first. (#1330) + # bslib 0.12.0 ## New features diff --git a/R/bs-dependencies.R b/R/bs-dependencies.R index 2ddd47e47..20b020136 100644 --- a/R/bs-dependencies.R +++ b/R/bs-dependencies.R @@ -298,6 +298,11 @@ bs_dependency <- function( #' that you may want to avoid memoisation if `func` relies on side-effects #' (e.g., files on-disk) that need to change for each themable widget #' instance. +#' @param cache_key A unique identifier for this dependency. Set this to a +#' distinct value (e.g. the widget name) whenever you create several +#' dependencies from similar `func`s. Without it, they can be mistaken for +#' one another in the cache, and every dependency after the first is served a +#' copy of the first one. Ignored when `memoise = FALSE`. #' #' @export #' @@ -351,7 +356,12 @@ bs_dependency <- function( #' myWidgetDependency() #' ) #' } -bs_dependency_defer <- function(func, memoise = TRUE) { +bs_dependency_defer <- function(func, memoise = TRUE, cache_key = NULL) { + if (!is.null(cache_key) && !memoise) { + rlang::warn("`cache_key` is ignored when `memoise = FALSE`.") + cache_key <- NULL + } + # func() most likely calls stuff like sass_file() and bs_dependency() -> # sass_partial() -> sass() (e.g., see example section above) Even though # sass() calls can be cached, there is still considerable overhead involved @@ -364,7 +374,17 @@ bs_dependency_defer <- function(func, memoise = TRUE) { # and then it is used once. This is not how memoized functions are normally # used, but in this case it works because the caching object is re-used, and # it still provides very significant improvement in performance. - mfunc <- memoise::memoise(func, cache = .dependency_cache) + memoise_func <- func + if (!is.null(cache_key)) { + # memoise() keys on formals, body, and args, not the enclosing + # environment, so fold cache_key in as a formal to tell otherwise + # identical closures apart. + formals(memoise_func) <- c( + formals(memoise_func), + list(.bslib_cache_key = cache_key) + ) + } + mfunc <- memoise::memoise(memoise_func, cache = .dependency_cache) } else { mfunc <- func } diff --git a/man/bs_dependency.Rd b/man/bs_dependency.Rd index 16363c1e2..c1429f084 100644 --- a/man/bs_dependency.Rd +++ b/man/bs_dependency.Rd @@ -15,7 +15,7 @@ bs_dependency( .sass_args = list() ) -bs_dependency_defer(func, memoise = TRUE) +bs_dependency_defer(func, memoise = TRUE, cache_key = NULL) } \arguments{ \item{input}{Sass rules to compile, using \code{theme}.} @@ -46,6 +46,12 @@ benefits when many instances of the same themable widget are rendered. Note that you may want to avoid memoisation if \code{func} relies on side-effects (e.g., files on-disk) that need to change for each themable widget instance.} + +\item{cache_key}{A unique identifier for this dependency. Set this to a +distinct value (e.g. the widget name) whenever you create several +dependencies from similar \code{func}s. Without it, they can be mistaken for +one another in the cache, and every dependency after the first is served a +copy of the first one. Ignored when \code{memoise = FALSE}.} } \value{ \code{bs_dependency()} returns an \code{\link[htmltools:htmlDependency]{htmltools::htmlDependency()}} and diff --git a/tests/testthat/test-bs-dependencies.R b/tests/testthat/test-bs-dependencies.R new file mode 100644 index 000000000..a0f9dfc19 --- /dev/null +++ b/tests/testthat/test-bs-dependencies.R @@ -0,0 +1,58 @@ +# A factory of deferred deps that differ only in captured variables (#1330). +defer_test_dep <- function(name, css, ...) { + bs_dependency_defer( + function(theme) { + if (!is_bs_theme(theme)) { + theme <- bs_theme(version = 5) + } + bs_dependency( + input = css, + theme = theme, + name = paste0("test-", name), + version = "0.0.0" + ) + }, + ... + ) +} + +test_that("bs_dependency_defer() gives factory-built closures distinct cache entries via `cache_key` (#1330)", { + red <- defer_test_dep("red", ".x { color: red }", cache_key = "red")() + blue <- defer_test_dep("blue", ".x { color: blue }", cache_key = "blue")() + + expect_identical(red$name, "test-red") + expect_identical(blue$name, "test-blue") +}) + +test_that("bs_dependency_defer() collides factory-built closures without `cache_key` (#1330)", { + red <- defer_test_dep("red", ".x { color: red }")() + blue <- defer_test_dep("blue", ".x { color: blue }")() + + # The deferred closures differ only in captured variables, so they share a + # memoise key: `blue` is served `red`'s cached dependency instead of its own. + expect_identical(red$name, "test-red") + expect_identical(blue$name, "test-red") +}) + +test_that("bs_dependency_defer(memoise = FALSE) recomputes on every call (#1330)", { + red <- defer_test_dep("red", ".x { color: red }", memoise = FALSE)() + blue <- defer_test_dep("blue", ".x { color: blue }", memoise = FALSE)() + + expect_identical(red$name, "test-red") + expect_identical(blue$name, "test-blue") +}) + +test_that("bs_dependency_defer() warns when `cache_key` is set with `memoise = FALSE`", { + expect_warning( + bs_dependency_defer(function(theme) NULL, memoise = FALSE, cache_key = "x"), + "cache_key.*ignored" + ) +}) + +test_that("bs_dependency_defer() keeps distinct functions separate without `cache_key`", { + dep_a <- bs_dependency_defer(function(theme) list(name = "a", version = "0.0.0"))() + dep_b <- bs_dependency_defer(function(theme) list(name = "b", version = "0.0.0"))() + + expect_identical(dep_a$name, "a") + expect_identical(dep_b$name, "b") +}) diff --git a/vignettes/custom-components/index.Rmd b/vignettes/custom-components/index.Rmd index 3258426f2..4a7247be8 100644 --- a/vignettes/custom-components/index.Rmd +++ b/vignettes/custom-components/index.Rmd @@ -109,6 +109,12 @@ person <- function(name, title, company) { Note that when `theme` is a `bs_theme()` object, then `person.scss` is compiled with Bootstrap Sass variables and mixins included via `bs_dependency()` (which returns the compiled CSS as an `htmlDependency()`). Otherwise, if `theme` is *not* a `bs_theme()` object, then `person()` is being used in a context where `{bslib}` is not relevant, so a pre-compiled CSS file is returned instead. Pre-complied CSS isn't necessarily a requirement, but it's a good idea for increasing performance and reducing software dependencies for end users. +::: {.callout .callout-note} +

Generating dependencies from a helper function

+ +Pass a distinct `cache_key`, such as the component `name`, when your dependency function is returned by another function. This keeps each dependency separate in the cache, instead of every one after the first inheriting the first's styles. +::: + ## HTML widgets For `{htmlwidgets}` that can be themed via CSS, we recommend supplying a `bs_dependency_defer()` to the `dependencies` argument of `createWidget()` (similar to the `person()` component from the last section), which will make the widget dynamically themeable. For widgets that can *not* be themed via CSS, the best option may be to query the active theme inside a `preRenderHook()` via `bs_current_theme()`, and then translate any relevant information to the widget's instance data, for example: