Avoid read/write in the event of a cache hit with non-NULL output

[cpsievert]

No description provided.

[schloerke]

Yes, we'll have to worry about utf8 reading and writing at some point. I wish we had this standardized instead of copying for the nth time.

[wch]

Notes:

• The return value when output is non-NULL should be consistent -- it shouldn't depend on whether there's a cache hit.
• Are you sure the change in return value (when output is non-NULL) won't cause breakage in packages that use sass?
• The docs need to be updated to reflect the change in return value.
• We should have some tests.
• We should think about turning on caching by default, at least for shiny.

[wch]

For writing UTF-8, here's what we ended up using in shinytest:
https://github.com/rstudio/shinytest/blob/f8bbbddd6c7c4e2e326b5ef2f738616a458561db/R/utils.R#L174-L177

Note that this does NOT automatically append a trailing \n. The version of the function from Shiny does add a \n if it's not present. The version in shinytest is more faithful to the input, so we should use it. Here's the commit where we changed it in shinytest:
rstudio/shinytest@5ec1208

[cpsievert]

The return value when output is non-NULL should be consistent -- it shouldn't depend on whether there's a cache hit.

Agreed, whenever output is non-NULL we return(invisible()) now (as we did prior to sass 0.2.0)

Are you sure the change in return value (when output is non-NULL) won't cause breakage in packages that use sass?

I'd be surprised if it does. sass has 4 reverse imports and the return(css) behavior was first introduced in sass 0.2.0 which hit CRAN in mid-March.

The docs need to be updated to reflect the change in return value.

The docs actually say it's return(invisible()):

sass/R/sass.R

Lines 20 to 22 in 0aa3469

	#' @return If \code{output = NULL}, the function returns a string value
	#' of the compiled CSS. If the output path is specified, the compiled
	#' CSS is written to that file and \code{invisible()} is returned.

When I changed it to return(css) when we implemented sass_layer(), I had forgot to update the docs (9d77894)

We should have some tests.

Just FYI, there are quite a bit of caching tests, but I'll look into adding a few more specifically for this logic

We should think about turning on caching by default, at least for shiny.

Ok, in that case, we'll have to find some way account for the bootstraplib version (bootstraplib themes include @imports of scss files from the installed packaged directory)