moduleServer() should allow passing a server function defined outside of itself

[ColinFay]

The new recommended way to write a module is the following:

name_ui <- function(id){
  ns <- NS(id)
  tagList(
    actionButton(ns("go"), "go")
  )
}

name_server <- function(id) {
  moduleServer(
    id,
    function(input, output, session) {
      observeEvent( input$go , {
        print("hey")
      })
    }
  )
}

This second argument to moduleServer being a function, it allows to extract this function and define it that way:

name_ui <- function(id){
  ns <- NS(id)
  tagList(
    actionButton(ns("go"), "go")
  )
}

name_server_core <- function(input, output, session) {
  observeEvent( input$go , {
    print("hey")
  })
}

name_server <- function(id) {
  moduleServer(
    id,
    name_server_core
  )
}

which IMHO is a cleaner way to write it if you have large module server functions.

The issue is that the current implementation of moduleServer() doesn't allow to pass argument to the module function. An alternative is to write:

name_ui <- function(id){
  ns <- NS(id)
  tagList(
    actionButton(ns("go"), "go")
  )
}

name_server_core <- function(input, output, session, arg) {
  observeEvent( input$go , {
    print(arg)
  })
}

name_server <- function(id, arg) {
  moduleServer(
    id,
    purrr::partial(name_server_core, arg = arg)
  )
}

But that might make the code a little bit more complex to read.

One good solution would be to be able to do:

name_server <- function(id, arg) {
  moduleServer(
    id,
    name_server_core, 
	arg = arg
  )
}

i.e. making moduleServer accept ....

Here is a modified version I wrote in one of my project that I use as a shim (and that would probably end as a shim in {golem}):

moduleServer <- function(
  id, 
  module, 
  ...,
  session = getDefaultReactiveDomain()
) {
  if (inherits(session, "MockShinySession")) {
    body(module) <- rlang::expr({
      session$setEnv(base::environment())
      !!body(module)
    })
    session$setReturned(callModule(module, id, session = session, ...))
  }
  else {
    callModule(module, id, session = session, ...)
  }
}

Let me know what you think. Happy to make a PR to implement that.

[jcheng5]

(Edit: tone came across grumpier than intended)

Hi Colin, thanks for the feedback!

It looks to me like this is exactly returning to callModule semantics. If you prefer callModule, you can just call it directly, instead of turning moduleServer back into callModule.

I think once you have broken your module server into core and wrapper, you've lost what was appealing about moduleServer in the first place: natural lexical binding of args, rather than introducing some args and passing through others in a single call.

I also don't see much appealing about a separate core function if you indent like I do:

name_server <- function(id) {
  moduleServer(id, function(input, output, session) {
    ...

This introduces only a single extra level of indentation, and no need to introduce another named function. I strongly prefer this tradeoff in my own code.

That said, I recognize this is an area of high subjectivity (which is one of the reasons there's always been so much pushback on the design of the module system in general).

[ColinFay]

Thanks! Indeed this is a subjective matter when it comes to indentation :)

I do like the callModule() function (probably because I'm very used to it, and it's always hard to change :) ), my only push for changing to the new version is the possibility to use moduleServer in tests.

One of the reason I like the old version of writing is that there was very few changes for the server side when you switch from "standard" to "module", making it easier to teach. What I mean by that is that this way to write the server is similar, in term of body, as a non modularized app:

name_server_core <- function(input, output, session, arg) {
  observeEvent( input$go , {
    print(arg)
  })
}

Whereas:

name_server <- function(id) {
  moduleServer(
    id,
    function(input, output, session) {
      observeEvent( input$go , {
        print("hey")
      })
    }
  )
}

is a little bit more complex to understand: you are writing a function that calls another function that defines an anonymous function inside it, so it's a little bit harder to explain/understand for beginners (I didn't run any real analysis for that point, just a general feeling when I switched to the new format in recent trainings).

I also have the general feeling that anonymous functions should generally be pure, and work the same way even if defined outside of the calling environment.

Anyway, I agree all this is a little bit subjective and subject to coding preference :)

The change I suggested in my first comment has a small breaking change (it changes the order of the arguments), but the function could also work the same way if ... was added as a last argument, inserting no breaking change in the current codebases (I think), and should probably be transparent for most user:

moduleServer <- function(
  id, 
  module, 
  session = getDefaultReactiveDomain(),
  ...
) {
  if (inherits(session, "MockShinySession")) {
    body(module) <- rlang::expr({
      session$setEnv(base::environment())
      !!body(module)
    })
    session$setReturned(callModule(module, id, session = session, ...))
  }
  else {
    callModule(module, id, session = session, ...)
  }
}

[jamieRowen]

I would like to support the proposal made by @ColinFay here, when developing an app in a package it feels nice to be able to have the core function separate with it's own documentation etc. I appreciate my view is also subjective but don't like the anonymous function definition inside a package function. Plus i like to describe the inputs, and outputs expected to be dealt with by the module in the documentation which feels more natural attached to a definition which has those as parameters.

Edit to add that my workaround for this is slightly different

name_server = function(id, arg) {
  environment(name_server_core) = environment()
  shiny::moduleServer(id, name_server_core)
}

[philibe]

I use both syntax:

• callModule() mainly for instanciated R6 sub parts used many times in my project,
• moduleServer() mainly for pages used one time in my project. I tried this syntax to learn it when it came out in shiny 1.5

With moduleServer() you can debug step into it without browser() but for the rest I prefer callModule() which seems to me, subjectively :), easier to write, and also I prefer parameters than scoped variables.

I write this post today because I've made today a sub sub module with moduleServer() which seems, subjectively :), less intuitive than other sub sub modules with callModule() that I 've done.

If you prefer callModule, you can just call it directly, instead of turning moduleServer back into callModule.

@jcheng5 : But you will never deprecate callModule() ? :) I've looked for an issue or thread about that when I read one more time in the doc:

"Starting in Shiny 1.5.0, we recommend using moduleServer instead of callModule()

Now I am here, I take this opportunity to a small question about data passed to submodule, especially not small dataframe:

• with scoped variable passed to moduleServer() you don't clone the variable ?
• with reactive dataframe passed by parameter in callModule() you don't clone the variable ?
• you create a new variable only for "normal" variable parameter in callModule() ?

( BTW Thank you again to have created Shiny. I've been using it for over 6 years now (2018) :) )