Fix #1359: shinyApp options argument ignored when passed to runApp

[bborgesr]

Fix #1359: shinyApp options argument ignored when passed to runApp.

The options arg to shinyApp() now works as expected, accepting the following shiny options:

• port
• launch.browser
• host

There are other params that runApp() accepts that shinyApp()'s options argument does not yet accept (these are workerId, quiet,display.mode). It may be worth having a discussion about whether any (or all, for consistency) of these should be promoted to a shiny option...

Here's the matrix of possibilities and the correspoding demo app:

1. no options are explicitly specified in shinyApp() or in runApp() -- defaults kick in (same behavior as before this PR):

library(shiny)
app <- shinyApp(
  ui = fluidPage(
    numericInput("n", "n", 1),
    plotOutput("plot")
  ),
  server = function(input, output) {
    output$plot <- renderPlot( plot(head(cars, input$n)) )
  },
)
runApp(app)

2. options are only explicitly specified in shinyApp() -- these should be followed (this did not work before this PR):

library(shiny)
app <- shinyApp(
  ui = fluidPage(
    numericInput("n", "n", 1),
    plotOutput("plot")
  ),
  server = function(input, output) {
    output$plot <- renderPlot( plot(head(cars, input$n)) )
  },
  options = list(launch.browser=rstudioapi::viewer, port=4000)
)
runApp(app)

3. options are only explicitly specified in runApp() -- these should be followed (same behavior as before this PR):

library(shiny)
app <- shinyApp(
  ui = fluidPage(
    numericInput("n", "n", 1),
    plotOutput("plot")
  ),
  server = function(input, output) {
    output$plot <- renderPlot( plot(head(cars, input$n)) )
  },
)
runApp(app, launch.browser=T, port=3000)

4. no options are explicitly specified both in shinyApp() and in runApp() -- options from runApp win (same behavior as before this PR):

library(shiny)
app <- shinyApp(
  ui = fluidPage(
    numericInput("n", "n", 1),
    plotOutput("plot")
  ),
  server = function(input, output) {
    output$plot <- renderPlot( plot(head(cars, input$n)) )
  },
  options = list(launch.browser=rstudioapi::viewer, port=4000)
)
runApp(app, launch.browser=T, port=3000)

[bborgesr]

Another note: as for workerId, quiet and display.mode: we don't need to make these shiny options in order for shinyApp()'s options argument to accept them and respect them. But it makes more sense to me to make them options and then do this than just doing this. This is mostly because the argument in question here is called options -- which is obviously reminiscent of shiny-options (at least, that is my impression).

I think this is a bit unfortunate because we don't want to allow all shiny-options to be able to be passed through shinyApp()'s options argument (right?? For this PR I worked under the assumption that we only wanted to allow port, launch.browser and host to be passed through here).

If the goal instead is that shinyApp()'s options argument accepts all argument that runApp() also accepts, then it should be called something different -- like runningOptions, or even just the familiar ... with the description "args to be passed on to runApp" (even if that is not exactly what we do under the hood). The downside of this, of course, is breaking backward compatibility... So we're not going to do it.

[bborgesr]

Hmm, so based on app.R line 373:

print.shiny.appobj <- function(x, ...) {
  opts <- x$options %OR% list()
  opts <- opts[names(opts) %in%
      c("port", "launch.browser", "host", "quiet", "display.mode")]

  args <- c(list(x), opts)

  do.call(runApp, args)
}

I think I've found my answer: we want to be able to pass "port", "launch.browser", "host", "quiet" and "display.mode", right? I think I'll make that clearer in the documentation for shinyApp()'s options param:

options Named options that should be passed to the runApp call (these can be any of the following: "port", "launch.browser", "host", "quiet" and "display.mode"). You can also specify width and height parameters which provide a hint to the embedding environment about the ideal height/width for the app.

Too bad we can't change the options name itself...

PS: I get that workerId is rarely used, but if we are allowing every other runApp() arg to be passed through, shouldn't we also allow it for this one?

[bborgesr]

So, in summary:

• The options arg to shinyApp() now works as expected, accepting the following runApp() params:

  • port
  • launch.browser
  • host
  • quiet
  • display.mode

• shinyApp's documentation improved (for the options arg -- the 5 params above are now explicitly mentioned)

[jcheng5]

LGTM.

[wch]

Are these missing() checks necessary? In general, it's best to avoid using missing(), because it can make it hard to call the function programmatically.

[wch]

I'm not 100% sure, but I don't think it's safe to move this line up here. This probably merits a discussion with @jcheng5.

[wch]

Do the strings "T" and "F" ever get passed in?

[wch]

I think the previous way, with the missing calls, had clearer intent. The match.call() and environment() assignment makes things kind of confusing. You could do something like this, where f is like runApp; x is like the app object; and, arg1 is an argument like port:

f <- function(x, arg1 = 1) {
  objValueOrDefault <- function(obj, name, default) {
    if (name %in% names(obj))
      obj[[name]]
    else
      default
  }
  
  if (missing(arg1)) arg1 <- objValueOrDefault(x, "arg1",  arg1)
  
  arg1
}

The drawback is that arg1 appears 4 times on one line, and that the if(missing()) part needs to be done for each argument. But it's easier to understand what it's doing.

It passes the following:

library(testthat)
expect_identical(f(list()),              1)
expect_identical(f(list(), 2),           2)
expect_identical(f(list(arg1 = 3)),       3)
expect_identical(f(list(arg1 = 3), 2),    2)
expect_identical(f(list(arg1 = NULL)),    NULL)
expect_identical(f(list(arg1 = NULL), 2), 2)

The one tricky case is in the fifth test: the object has arg1=NULL, and that NULL gets used. I think that behavior makes the most sense.

If we didn't do things that way (if it returned f's default value for arg1, which is 1), then f could be simplified:

f <- function(x, arg1 = 1) {
  `%OR2%` <- function(x, y) {
    if (!is.null(x)) x
    else             y
  }
  
  if (missing(arg1)) arg1 <- x$arg1 %OR2% arg1
  
  arg1
}

[wch]

Would the name appOpts make more sense here? Because these are options from the app object (as opposed to those given to runApp).

[wch]

This function doesn't assign values, so maybe a name like defaultVal would be better. Or maybe valueDefault?