Skip to content
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

Enhancement (?) #81

Open
nhward opened this issue Jul 11, 2024 · 10 comments
Open

Enhancement (?) #81

nhward opened this issue Jul 11, 2024 · 10 comments

Comments

@nhward
Copy link

nhward commented Jul 11, 2024

I can create and run guides at a module level. By this I mean I can define and init/start conductor guides within a module (provided I am careful to namespace the ids. This works great since I can manage the module and the guide together. This is excellent news!

However, the reactivity associated with the guide does not work in this mode. For example. the guide$isActive() always returns NULL because the underlying session input does not exist (not by the expected name or any other names). See REPREX.
I gather that the guide(s) must be initialised in the "main" server for the input values to be created and work correctly to give the necessary reactivity. I am not sure why this is. This limitation is not discussed in the documentation.

My questions are these: Is this behaviour by design or an oversight? Is it simple to enable the session$inputs to be created from within modules?

Below is some code I borrowed from the documentation with the initialisation and start functions triggered within the module.

library(shiny)
library(conductor)

guide <- Conductor$new()$
  step(
    title = "hello there"
  )


# First module

mod_num_ui <- function(id){
  ns <- NS(id)
  numericInput(ns("num"), label = "Enter a number", value = 5, min = 0, max = 10)
}


mod_num_server <- function(input, output, session){
  ns <- session$ns
  guide$
    step(
      el = ns("num"), # use namespace
      title = "Step 1",
      text = "This is a numericInput."
    )

  observeEvent(
    guide$isActive(),
    {
      print(guide$isActive())   ### This never gets printed
    }
  )

  # launch after module called
  guide$init()$start()
}

# Second module

mod_text_ui <- function(id){
  ns <- NS(id)
  textInput(ns("text"), label = "Type some text")
}

mod_text_server <- function(input, output, session){
  ns <- session$ns
  guide$
    step(
      el = ns("text"),
      title = "Step 2",
      text = "This is a textInput."
    )
}

# Main app

ui <- fluidPage(
  useConductor(),
  mod_num_ui("num1"),
  mod_text_ui("text1")
)

server <- function(input, output, session){

  callModule(mod_num_server, "num1")
  callModule(mod_text_server, "text1")
  print(input)  ### there are no guide related input entries
}

shinyApp(ui = ui, server = server)
@etiennebacher
Copy link
Owner

Hi @nhward, thanks for the question and the reproducible example, I think this should work. I took a quick try and didn't figure out why it doesn't, but I'll try again this weekend.

@etiennebacher
Copy link
Owner

I have found a solution. I will first put the code that works and then go into a bit more detail afterwards.

Code that works:

library(shiny)
library(conductor)

guide <- Conductor$new()$
  step(
    title = "hello there"
  )


# First module

mod_num_ui <- function(id){
  ns <- NS(id)
  numericInput(ns("num"), label = "Enter a number", value = 5, min = 0, max = 10)
}


mod_num_server <- function(input, output, session, global_session){
  ns <- session$ns
  
  guide$
    step(
      el = ns("num"), # use namespace
      title = "Step 1",
      text = "This is a numericInput."
    )
  
  observeEvent(
    guide$isActive(session = global_session),
    {
      print(guide$isActive(session = global_session))   ### This never gets printed
    }
  )
  
  guide$init(session = global_session)$start(session = global_session)
}

# Second module

mod_text_ui <- function(id){
  ns <- NS(id)
  textInput(ns("text"), label = "Type some text")
}

mod_text_server <- function(input, output, session){
  ns <- session$ns
  guide$
    step(
      el = ns("text"),
      title = "Step 2",
      text = "This is a textInput."
    )
}

# Main app

ui <- fluidPage(
  useConductor(),
  mod_num_ui("num1"),
  mod_text_ui("text1")
)

server <- function(input, output, session){  
  callModule(mod_num_server, "num1", global_session = session)
  callModule(mod_text_server, "text1")
}

shinyApp(ui = ui, server = server)

Explanation:

When we create a guide, we assign it a unique ID, for example aefzrgza. This ID is the same no matter where the guide is created or updated. You can add $step() in a module, it won't change the guide's ID. Internally, we set an input value that has the form aefzrgza_is_active and which returns TRUE or FALSE. When we call $isActive(), we look for the value of aefzrgza_is_active in the session.

As you noticed, this works fine when we call it in the server function, but not in modules. The reason is that the session argument in a module is a child of the session argument in the server. Therefore, while aefzrgza_is_active exists in the "main" session, it doesn't exist in the "child" session.

The solution is to pass the "main" session as an additional argument to the module. In the example above, I add an argument global_session to the module and it takes the value of the "main" session in server. Therefore, once we are in the module, we have access to the values in the "main" session and therefore we can check whether the guide is active or not.


@nhward those explanations are maybe a bit too technical. Let me know if the code I suggested works. If you can adapt this solution to your real code, then I will add a section about this in the docs. Otherwise, feel free to ask any questions.

@nhward
Copy link
Author

nhward commented Jul 16, 2024

Etienne,
The problem fundamentally arises (I think) because you do not allow the user the option to supply a unique id to the "guide". You always create a unique id that is unaware of the current session name-space.
If you did (optionally) allow the ability to specify the id ( e.g. guide <- conductor$new(id = session$ns("myId") ) then (I believe) the name-spacing can all be taken care of.

I am particularly interested in the scenario whereby the guide is created within a module, and run within the module. All your examples assume the guide is part of the global environment - hence your solution above. I am using package TidyModules to manage the modules via a module class. I highly recommend this package. This is an excellent package but its design does not easily allow me to call call module() with extra parameters. There are other ways to access the global session that I will look into.

In modules the following pattern is normal:

  1. In UI: create inputs with id = ns("myId")
  2. In Server: refer to inputs as "myId" alone

By exclusively managing the guide id internally you are breaking this pattern. Specifically, step 1 cannot be achieved. Some third-party shiny-support packages (e.g. shinyjs) utilise the asis = TRUE approach to allowing the namespace-free id to be employed in the server code.

I hope this discussion helps create a universal solution. I reckon exposing an optional id parameter in the $new() function is the best approach. I am happy to test such a solution.

@nhward
Copy link
Author

nhward commented Jul 17, 2024

I can confirm that your fix does work. Rather than passing a global_session parameter as you suggested, I have employed the use of session$userData

"Root" server code:

session$userData$RootSession <- session

Module code:

guide <- NULL

observe({
   guide$new()
   guide$step(...)
   guide$init(session = session$userData$RootSession)
})

observeEvent(
  input$GuideButton,
  {
    req(guide)
    guide$start(session = session$userData$RootSession)
  }
)

observe({
  req(guide)
  if (guide$isActive(session = session$userData$RootSession)) {
       etc
  }
})

The expected behavior is now working.

@nhward
Copy link
Author

nhward commented Jul 17, 2024

Some additional comments:

I noticed that methods isActive() and getCurrentStep() are triggering too often. I am not sure why this is. I tried wrapping these as a reactive expression but this did not fix the problem. My application responsiveness is lost due to the eternal loop of rechecking whether the Conductor is active.

From my experimentation:

  • The getCurrentStep() method triggers on the step being left.
  • The isOpen() method triggers on the step being entered.
    This subtly is missing from the documentation.

@etiennebacher
Copy link
Owner

etiennebacher commented Jul 17, 2024

The problem fundamentally arises (I think) because you do not allow the user the option to supply a unique id to the "guide". You always create a unique id that is unaware of the current session name-space.

That was actually quite on purpose, so that one can extract the guide (or at least part of it) from the ui and server parts, leading to less cluttered code.

I am particularly interested in the scenario whereby the guide is created within a module, and run within the module.

I thought that you could simply create a new guide there, but it seems it doesn't work while it should. Here's a (shorter) example:

library(conductor)
library(shiny)

guide <- Conductor$new()$
  step(
    title = "hello there"
  )

mod_num_ui <- function(id){
  ns <- NS(id)
  numericInput(ns("num"), label = "Enter a number", value = 5, min = 0, max = 10)
}

mod_num_server <- function(input, output, session){
  ns <- session$ns
  
  new_guide <- Conductor$new()$
    step(
      el = ns("num"),
      title = "Step 1",
      text = "This is a numericInput."
    )
  
  new_guide$init(session = session)

  observeEvent(
    new_guide$isActive(session = session),
    {
      print(new_guide$isActive(session = session))  
    }
  )
  
  new_guide$start(session = session)
}

ui <- fluidPage(
  useConductor(),
  mod_num_ui("num1")
)

server <- function(input, output, session){  
  callModule(mod_num_server, "num1")
}

shinyApp(ui = ui, server = server)

I need to fix that but I think this should answer your request. Basically the problem arises when the session in which $init() is run is different from the one in which $isActive() is run. If you want to run some of those parts in different modules then they all need to share the same session object, and I don't see how adding an id argument in $new() would help.

If you could share a (non-working) example of what you'd like your code to look like that would be great.

Some additional comments

Thanks, I moved those to another issue to keep this one about your first issue only.


PS: I haven't used shiny in a while now so I might be missing some info on recent changes in good practices and other features.

@etiennebacher
Copy link
Owner

etiennebacher commented Jul 17, 2024

Here's why the code in my previous post doesn't work: https://stackoverflow.com/a/59013515

However using sendInputMessage would require creating a binding for the guide, and I don't really know how to do that so it probably won't happen soon. I'll look for other solutions.

@nhward
Copy link
Author

nhward commented Jul 17, 2024

You wrote:

Basically the problem arises when the session in which $init() is run is different from the one in which $isActive() is run.

This would imply that if these are both the module-session or these are both the root-session then $isActive() would work. In my testing, only when the root-sessions are used throughout, do things work correctly.

My (somewhat naive) opinion is that this is because the module-sessions (somehow) translate all use of ids in the server code into namespaced ids. Since you create the private$id of (say) "prebtchaioskjdqngfyvmxuzlw", isActive() might be inadvertently be dealing with "mod1-prebtchaioskjdqngfyvmxuzlw_is_active" rather than "prebtchaioskjdqngfyvmxuzlw_is_active".

The code below can be easily switched between module-session and root-session by uncommenting some code.

library(conductor)
library(shiny)

######### start of module
mod_num_ui <- function(id){
  ns <- NS(id)
  tagList(
    numericInput(inputId = ns("num"),  label = "Enter a number", value = 5, min = 0, max = 10),   #true id = "mod1-num"
    actionBttn(  inputId = ns("tour"), label = "start Tour")  # true id = "mod1-num"
  )
}

mod_num_server <- function(input, output, session) {
  ns <- session$ns

  sess <- session$userData$rootSession   # use root session
  #sess <- session                       # use module session
  
  guide <- Conductor$new()$
    step(
      el = ns("num"),  #translated id = "mod1-num"
      title = "Step 1",
      text = "This is a numericInput."
    )

  guide$init(session = sess)

  observeEvent(
    guide$isActive(session = sess),
    {
      print(guide$isActive(session = sess))
    }
  )

  observeEvent(
    input$tour,
    {
      guide$start(session = sess)
      print(sess$input)
      print(names(sess$input))
      browser()
    }
  )
}

########## end of module

ui <- fluidPage(
  useConductor(),
  mod_num_ui("mod1")
)

server <- function(input, output, session){
  session$userData$rootSession <- session
  callModule(mod_num_server, "mod1")
}

shinyApp(ui = ui, server = server)

When I run the example app above with the module session, and inspect $input, I get:

Values: mod1-num, mod1-tour, prebtchaioskjdqngfyvmxuzlw_is_active
names: "tour" "num"

When I run the example app above with the root-session, and inspect $input, I get:

Values: mod1-num, mod1-tour, prebtchaioskjdqngfyvmxuzlw_is_active
names: "mod1-tour" "mod1-num" "prebtchaioskjdqngfyvmxuzlw_is_active"

Two things are clear:

  1. The full range of ids are available in both the root-session and the module-sessions (i.e. there is a single set of js element ids). These sessions are not independent.
  2. The names(input) somehow hides ids that are not part of the session's namespace.

I wonder if session$sendCustomMessage() (which lies inside of $isActive()) is likewise affected by the session's namespace.

If the Conductor's id is also namespaced, perhaps its visibility will allow '$isActive()' to work normally in a module without resorting to root-sessions. This is pure speculation but not something I can experiment with (because the Conductor id is private)

Not withstanding, there is still the issue of binding the *_is_active id to shiny in a reactive fashion. It is just possible that these issues have overlap.

@etiennebacher
Copy link
Owner

I don't think the problem is related to the name given to the id. The issue is that, as mentioned in this SO answer:

By the time you call Shiny.setInputValue() in JS, you've already lost all information about the session: only the message data remains.

So it doesn't matter whether we $init() the guide in the root or in the module, the input value that is created (<guide_id>_is_active) will be stored in the root session. And then the module doesn't have access to the list of inputs stored in this session, unless we explicitly access it with session$userData$rootSession.

I will look more into sendInputMessage(), but if you have some knowledge on that I would gladly review a PR.

@nhward
Copy link
Author

nhward commented Jul 20, 2024

Etienne, I am well out of my depth to give much advice. I can see from here that sendInputMessage() is namespace aware even if sendCustomMessage() is not.

This post here discusses the differences between the two calls.

You wrote:

And then the module doesn't have access to the list of inputs stored in this session, unless we explicitly access it with...

I this is not totally true. The point of my earlier comment was to show that "prebtchaioskjdqngfyvmxuzlw_is_active" was present in both the module and root session lists. I suspect the only way to reliably access the out-of-namespace ids in R is through sess$input["prebtchaioskjdqngfyvmxuzlw_is_active"]. Please see if you can confirm this.

Going by your recent code changes, the documentation would need to inform users to add

session$userData$rootSession <- session

to their root-session when using modules. Perhaps programmatically warn if this is being relied upon but not set, e.g. 'Warning: session is NULL, add line "session$userData$rootSession <- session" to the main server code.'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants