-
Notifications
You must be signed in to change notification settings - Fork 137
Description
Thanks for creating this package, it is definitely filling a gap that has existed in the Shiny ecosystem for too long!
I recently took a closer look at golem and a couple of things surprised me, which turned out to be related. My first question was why golem_opts was necessary, and the second was how you could pass arguments from run_app to app_ui or app_server, which it looks like was the whole reason for golem_opts--please correct me if I'm wrong here.
My instinct would have been to change the run_app pattern to look more like (ok, exactly like) app.R:
# Instead of `...`, add whatever explicit parameters you need
run_app <- function(csv_path = system.file("data/example.csv", package="mypkg")) {
df <- read.csv(csv_path)
ui <- fluidPage(
selectInput("xvar", "x", colnames(df), colnames(df)[[1]]),
selectInput("xvar", "y", colnames(df), colnames(df)[[2]]),
...
)
server <- function(input, output, session) {
r <- reactive({
df[, c(input$x, input$y)]
])
...
}
shinyApp(ui, server)
}This nicely preserves Shiny users' expectation to be able to create variables at the top of server.R that's shared between sessions. Is this an approach you considered and rejected?
If you are dead set on maintaining app_ui and app_server in separate functions/files, you still could do away with golem_opts by using purrr::partial:
run_app <- function(...) {
shinyApp(
ui = app_ui,
server = purrr::partial(app_server, !!!...)
)
}
# elsewhere...
app_server <- function(input, output, session, csv_path) { ... }This would have the significant advantage of giving app_server a function signature that says right up front what parameters it expects from run_app.
Two other nits:
- I'd encourage people to put formal parameters in
run_apprather than... - This is mostly me being uptight but as I'm sure you know,
run_appdoesn't actually run the app; it just creates it, it's the printing that causes it to be run. You can also do other things with that app object besides printing it, including iframe-embedding it within another, larger shiny app (admittedly this is not a super common usage). I thinkrun_apphas the right behavior but I'd personally pick a different, more noun-oriented name.
Thanks again for your work, I hope you find this feedback useful.