fix: Only copy pre-compiled themes once#1184
Merged
Conversation
gadenbuie
commented
Feb 11, 2025
R/bs-dependencies.R
Outdated
| } | ||
|
|
||
| version <- theme_version(theme) | ||
| deps_hash <- rlang::hash(htmlDependencies(as_sass(theme))) |
Member
Author
There was a problem hiding this comment.
This is probably over-thinking things. The one way that the theme could change between calls is if the Bootstrap JavaScript assets change. Taking the hash of the dependencies was intended to catch this, but we'd actually have to hash the files themselves. Not sure if it's worth it.
cpsievert
approved these changes
Feb 11, 2025
cpsievert
approved these changes
Feb 16, 2025
Contributor
|
Just FYI: This also seems to fix a performance problem with the latest CRAN version (0.9.0) of bslib: |
6 tasks
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
3 participants
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.
Currently, when a pre-compiled theme is used, we re-use
{tempdir()}/bslib-precompiled-5but copy files from bslib to the temp directory every time the pre-compiled theme is used.This PR refactors
bs_theme_dependencies()to pull out the core logic (either use a precompiled theme or compile the theme via Sass) into helper functions, which clarifies the flow.Then, if we've already prepared the full dependency in the temp directory, we re-use the prepared dependency on subsequent calls to
bs_theme_dependencies().Closes #1154. The issue seen there arises when we copy files out of the package but the package is stored with all files having read-only (400) permissions. This doesn't cause problems until
bs_theme_dependencies()is called for a second time, where we end up trying to overwrite existing files. This is a partial fix in the sense thatnixwill still complain about being unable to remove temp files, but we can't solve that entirely from bslib anyway.