-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add SHINYLIVE_WASM_PACKAGES
environment variable
#116
Conversation
0fe70aa
to
123e9a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to handle it in this PR, but we're using enough environment variables now that I think we should formally document them in their own section in the docs.
R/quarto_ext.R
Outdated
wasm_packages_val <- sys_env_wasm_packages() | ||
wasm_packages <- as.logical(wasm_packages_val) | ||
if (is.na(wasm_packages)) { | ||
cli::cli_abort(c( | ||
"x" = "Could not parse `wasm_packages` value: {.code {wasm_packages_val}}" | ||
)) | ||
wasm_packages <- SHINYLIVE_WASM_PACKAGES | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth putting this in a helper function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I realised we're only really expecting to see an NA
when we're parsing the string from Sys.getenv()
, so I've moved the check and cli_abort()
into the sys_env_wasm_packages()
function, avoiding the duplication.
Sounds good, I'll add an issue along those lines once this has been merged. |
I've also added a line in NEWS.md. Will also directly add a NEWS entry for #114 once this is merged. |
ed4bb73
to
48ee638
Compare
#' @param wasm_packages Download and include binary WebAssembly packages as part | ||
#' of the output app's static assets. Logical, defaults to `TRUE`. The default | ||
#' value can be changed by setting the environment variable | ||
#' `SHINYLIVE_WASM_PACKAGES` to `TRUE` or `1` to enable, `FALSE` or `0` to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's annoying, but I think we should allow literally "true"
or "false"
, or the whole range of TRUE
, "TRUE"
, "true"
, "1"
and 1
and false variants. (Basically, `tolower(val) %in% c("true", "1").)
I would read this expecting I could set SHINYLIVE_WASM_PACKAGES=TRUE
in .Renviron
, but IIRC that will end up being "TRUE"
when set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be handled OK by:
pkgs_env <- switch(pkgs_env, "1" = TRUE, "0" = FALSE, pkgs_env)
wasm_packages <- as.logical(pkgs_env)
Because "1" will be caught by the switch
, and then "TRUE"
/"true"
will be caught by the as.logical()
.
> as.logical("TRUE")
[1] TRUE
> as.logical("true")
[1] TRUE
> as.logical(TRUE)
[1] TRUE
> as.logical("false")
[1] FALSE
> as.logical("something-weird")
[1] NA
Saying that, I might change it just to make things clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice, that makes sense. I just missed that as.logical()
line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I do think it's worth adjusting the values accepted for SHINYLIVE_WASM_PACKAGES
Continuing the work in #114 and earlier PRs, adds an environment variable
SHINYLIVE_WASM_PACKAGES
, defaulting toTRUE
.When
SHINYLIVE_WASM_PACKAGES
is set toFALSE
or0
, no wasm R packages are exported at all. This allows the user to turn off the mechanism when using Shinylive with Quarto, should they want to.Closes quarto-ext/shinylive#60.