fix(pkg-r): Directly expose id rather than ask for ns() function

[gadenbuie]

This stacks on #172 and is an alternate approach that exposes id.

The ns argument in #172 is convenient, but it's also not a standard a pattern that we use in Shiny modules. In general, it's normal to expect that you'll need to wrap any id for any component in ns(), e.g. id = ns("component"), in a Shiny module, including when you're nesting Shiny modules.

Passing in ns is an interesting idea, but it's not the standard established pattern and I'm not sure it's entirely worth it because it doesn't entire enable another class of use cases.

Allowing users to pass in id, on the other hand, is idiomatic and expected and gives users extra flexibility to call $ui() and $server() with alternate IDs, even in a non-module use case. That's entirely valid because we've decoupled the $server() return values from the parent QueryChat class.

Example

Here's the example in #172 updated to use the new pattern.

library(shiny)
library(bslib)
library(querychat)

qc <- QueryChat$new(mtcars, greeting = "Ask me about mtcars!")

module_ui <- function(id, qc) {
  ns <- NS(id)
  card(qc$ui(id = ns("qc-ui"))) #<< this is normal Shiny module stuff
}

module_server <- function(id, qc) {
  moduleServer(id, function(input, output, session) {
    qc$server(id = "qc-ui") #<< also expected in a Shiny module
  })
}

ui <- page_fluid(
  titlePanel("QueryChat Module Test"),
  module_ui("module1", qc)
)

server <- function(input, output, session) {
  module_server("module1", qc)
}

shinyApp(ui, server)

So while the new R6 class saves you from needing to pass IDs around when you're using qc$ui() and qc$server() in a normal Shiny app (and you only have one), if you're in Shiny modules or if you have more than one chat from the same qc source object, you'll be able to connect the ids by passing them explicitly.

[cpsievert]

Allowing users to pass in id, on the other hand, is idiomatic and expected

I feel a bit torn on how true this is. It is idiomatic if you know that $ui()/$server() functions are module functions, but how obvious is that? And how familiar of a pattern is it to call a module from within another module?

The ns argument in #172 is convenient, but it's also not a standard a pattern that we use in Shiny modules

The class based approach is already non-standard, so I'm not sure following standards is obviously more helpful, especially when the standard is rooted in semi-internal implementation details

Ultimately, I do feel a bit conflicted on this. I do agree in part with stance, but the aspect that pushed me towards ns was the fact that it is confusing for the instance to have an id that you can also override into irrelevance with $ui() and $server(). With ns, there is no confusion over when/which ids to set, and it's more obvious that it's only there because of the modules use case.

[gadenbuie]

Allowing users to pass in id, on the other hand, is idiomatic and expected

I feel a bit torn on how true this is. It is idiomatic if you know that $ui()/$server() functions are module functions, but how obvious is that? And how familiar of a pattern is it to call a module from within another module?

It's idiomatic in a few ways, most notably that in Shiny modules anywhere that you would ordinarily give an id you have to wrap it in ns() in UI code. That's a basic rule of writing Shiny modules that everyone understands you need to follow. I can't think of an example where we've recommended passing in ns as an alternative approach. So it's idiomatic in the sense that you could understand how to write Shiny modules, know nothing about what we're doing in QueryChat and write correct code.

It's also idiomatic in the sense that we literally outline this exact scenario in our modules article, succinctly enough that I can quote it entirely here

Nesting modules

Modules can use other modules. When doing so, when the outer module’s UI function calls the inner module’s UI function, ensure that the id is wrapped in ns(). In the following example, when outerUI calls innerUI, notice that the id argument is ns("inner1"):

innerUI <- function(id) {
  ns <- NS(id)
  "This is the inner UI"
}

outerUI <- function(id) {
  ns <- NS(id)
  wellPanel(
    innerUI(ns("inner1"))
  )
}

As for the module server functions, just ensure that the call to callModule for the inner module happens inside the outer module’s server function. There’s generally no need to use ns().

innerServer <- function(id) {
  moduleServer(
    id,
    function(input, output, session) {
      # inner logic here
    }
  )
}

outerServer <- function(id) {
  moduleServer(
    id,
    function(input, output, session) {
      innerResult <- innerServer("inner1")
      # outer logic here
    }
  )
}

Ultimately, I do feel a bit conflicted on this. I do agree in part with stance, but the aspect that pushed me towards ns was the fact that it is confusing for the instance to have an id that you can also override into irrelevance with $ui() and $server().

We have enough examples of this already where you can set a value in the constructor that can later be overridden in specific methods. It's less "override into irrelevance" and more "convenience by default".

The approach in this PR benefits two use cases:

1. You're putting querychat in a module
2. You want two separate chat instances for the same data source

The first use case is covered by both PRs, but the second case is much trickier to set up correctly if you can't access id directly. Both use cases are very much in the long tail of users, but this indicates to me that passing in ns isn't quite at the right point in the convenience ←→ customizability spectrum.