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

Fix regression in false positive caching of sass_file() #85

Merged
merged 7 commits into from Apr 29, 2021

Conversation

cpsievert
Copy link
Contributor

Closes #84

@cpsievert cpsievert marked this pull request as draft April 29, 2021 17:53
Dependencies shouldn't influence Sass caching and nowadays they're always attached to the return value
@cpsievert cpsievert marked this pull request as ready for review April 29, 2021 21:48
@cpsievert
Copy link
Contributor Author

When we recently started having as_sass() return HTML deps (#74), that should have broken this test expectation

# Make sure cache key doesn't change with new/temporary HTML dependencies
my_layer <- function() {
src <- tempfile()
dir.create(src)
sass_layer(
"@function fib($x) {
@if $x <= 1 {
@return $x
}
@return fib($x - 2) + fib($x - 1);
}
body { width: fib(27);}",
html_deps = htmltools::htmlDependency("foo", "1.0", src)
)
}
expect_cached(
my_layer(),
"body{width:196418;}"
)

Turns out that test wasn't working the way I intended it to work when I added it in #63, so
d1b55e3 fixes that issue

@cpsievert cpsievert merged commit e60dd7e into master Apr 29, 2021
@cpsievert cpsievert deleted the sass-file-cache-bugfix branch April 29, 2021 22:06
} else if (is_font_collection(x)) {
x$html_deps <- NULL
} else {
htmlDependencies(x) <- NULL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this won't remove an html dependency if it's included as a child of a tag object (as opposed to being added with attachDependency()):

x <- div(
  "hello",
  htmlDependency("x", "1.0", c(src = "abc"))
)
htmlDependencies(x) <- NULL
findDependencies(x)
#> [[1]]
#> List of 10
#>  $ name      : chr "x"
#>  $ version   : chr "1.0"
#>  $ src       :List of 1
#>   ..$ src: chr "abc"
#>  $ meta      : NULL
#>  $ script    : NULL
#>  $ stylesheet: NULL
#>  $ head      : NULL
#>  $ attachment: NULL
#>  $ package   : NULL
#>  $ all_files : logi TRUE
#>  - attr(*, "class")= chr "html_dependency"

Same is true if it's an element in a tagList or list:

x <- tagList(
  div("hello"),
  htmlDependency("x", "1.0", c(src = "abc"))
)
htmlDependencies(x) <- NULL
findDependencies(x)
#> [[1]]
#> List of 10
#>  $ name      : chr "x"
#>  $ version   : chr "1.0"
#>  $ src       :List of 1
#>   ..$ src: chr "abc"
#>  $ meta      : NULL
#>  $ script    : NULL
#>  $ stylesheet: NULL
#>  $ head      : NULL
#>  $ attachment: NULL
#>  $ package   : NULL
#>  $ all_files : logi TRUE
#>  - attr(*, "class")= chr "html_dependency"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is an actual issue since as_sass() currently doesn't have a method for tags for dependencies (this is partially the motivation for having the sass_layer(html_deps)):

> sass::as_sass(list(".foo{}", htmltools::htmlDependency("foo", "1.0", ".")))
Error in as_sass_.default(val) : 
  as_sass() not implemented for object of class: 'html_dependency'

That being said, maybe it still make sense to add a case:

} else if (inherits(x, "html_dependency")) {
    x <- NULL
  }

just in case we decide to add a no-op method someday

cpsievert added a commit that referenced this pull request Apr 30, 2021
…dependencies(). This case is actually relevant given that as_sass() doesn't currently have a method for dependencies, but it's possibly that someday we'll add one

https://github.com/rstudio/sass/pull/85/files\#r623952380
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using sass_file() can lead to false positive caching
2 participants