Fix bs_dependency_defer() cache collisions for factory-built closures

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

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
   }

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")
+})

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}
+<h3 data-toc-skip>Generating dependencies from a helper function</h3>
+
+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: