Fix bs_dependency_defer() cache collisions for factory-built closures - #1348
Open
AleKoure wants to merge 2 commits into
Open
Fix bs_dependency_defer() cache collisions for factory-built closures#1348AleKoure wants to merge 2 commits into
AleKoure wants to merge 2 commits into
Conversation
…sions `bs_dependency_defer()` memoises `func` against a shared cache keyed on its formals, body, and call arguments -- but not its enclosing environment. Closures produced by a factory (the pattern from the "Dynamically themeable component" article) differ only in captured variables, so they hash to the same key and every dependency after the first is served the first one's compiled output. Add an optional `cache_key` argument that is folded into the memoise key so each factory-built closure gets its own cache entry. Behavior is unchanged when `cache_key` is NULL, and the argument is ignored (with a warning) when `memoise = FALSE`. Fixes rstudio#1330 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…encies Add a callout to the "Dynamically themeable component" article explaining that `bs_dependency_defer()` needs a distinct `cache_key` when the dependency function is returned by another function, so each dependency stays separate in the cache. Also trim an overly verbose comment in the new tests. Re rstudio#1330 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1330
Summary
bs_dependency_defer()memoises dependency functions using a shared cache. Thememoise key includes the function's formals and body, but not its enclosing
environment.
As a result, closures created by the same factory can collide when they differ
only in captured values. After the first dependency is cached, subsequent
dependencies may incorrectly receive the first dependency's compiled output.
This PR adds an optional
cache_keyargument:When supplied,
cache_keyis folded into the memoise key, allowing callers todistinguish factory-generated dependency functions. Existing behavior is
unchanged when
cache_key = NULL.When
memoise = FALSE,cache_keyhas no effect and a warning is emitted.Reproducible example
Without distinct cache keys, both factory-generated closures have identical
formals and bodies and can resolve to the same cached dependency.
Documentation
The
bs_dependency_defer()documentation now describes whencache_keyisneeded. The custom-components vignette also notes that helper-generated
dependency functions should use a stable, distinct cache key.
Testing
Added unit tests covering:
cache_key;cache_key;memoise = FALSE;cache_keyis combined withmemoise = FALSE;devtools::check()completes with 0 errors, 0 warnings, and 0 notes.Checklist
tests/testthat.devtools::check()passes.NEWS.mdincludes an entry for the change.