Appsilon / shiny.router

A minimalistic router for your Shiny apps.
http://appsilon.github.io/shiny.router
Other
255 stars 31 forks source link

observeEvent triggered as many times as page_change #71

Closed andbamp closed 4 years ago

andbamp commented 4 years ago

Hello, thanks for your useful package!

I have run into an issue when it comes to changing pages. Here is a minimal example:

library(shiny)
library(shiny.router)

# This generates menu in user interface with links.
menu <- (
  tags$ul(
    tags$li(a(class = "item", href = route_link("home"), "Home page")),
    tags$li(a(class = "item", href = route_link("side"), "Side page"))
  )
)

# This creates UI for each page.
page <- function(title, content, button_name) {
  div(
    menu,
    titlePanel(title),
    p(content),
    actionButton("switch_page", "Click to switch page!"),
    actionButton(button_name, "Go!")
  )
}

# Both sample pages.
home_page <- page("Home page", uiOutput("current_page"), "test1")
side_page <- page("Side page", uiOutput("current_page"), "test2")

home_server <- function(input, output, session) {
  observeEvent(input$test1, {
    print("Foo")
  })
}

side_server <- function(input, output, session) {
  observeEvent(input$test2, {
    print("Bar")
  })
}

# Creates router. We provide routing path, a UI as
# well as a server-side callback for each page.
router <- make_router(
  route("home", ui = home_page, server = home_server),
  route("side", ui = side_page, server = side_server)
)

# Create output for our router in main UI of Shiny app.
ui <- shinyUI(fluidPage(
  router_ui()
))

# Plug router into Shiny server.
server <- shinyServer(function(input, output, session) {
  router(input, output, session)

  output$current_page <- renderText({
    page <- get_page(session)
    sprintf("Welcome on %s page!", page)
  })

  observeEvent(input$switch_page, {
    if (is_page("home")) {
      change_page("side")
    } else if (is_page("side")) {
      change_page("home")
    }
  })
})

# Run server in a standard way.
shinyApp(ui, server)

Expected behaviour:

Actual behaviour:

This seems to be related to issue #67. Any feedback is appreciated!

krystian8207 commented 4 years ago

@andbamp This is the behavior that happens when you use observers inside of the page. This is because shiny doesn't destroy observers when the server code is called for the n-th time but registers them again (please check my blogpost that describes the issue: https://appsilon.com/how-to-safely-remove-a-dynamic-shiny-module/).

Unfortunately in shiny.router, the main functionality is based on rerendering the page server code. We've got a few ideas how to implement this properly but this is of a low priority now, but I'll let you know when it's done.

In the meantime, you may try to force destroying observers (following instructions from the above blogpost, my solution in #69 might also help) everytime the page is switched. Please let me know if you need any help with it.

krystian8207 commented 4 years ago

@andbamp I decided to give my idea a try, and the issue should be fixed now. Please install the package from https://github.com/Appsilon/shiny.router/tree/krystian.stop-rerendering-whole-page branch and let me know if everything works correctly now.

andbamp commented 4 years ago

I didn't realise shiny.router's functionality was based on rerunning the server code. This fix not only solves the issue of console output being printed multiple times (and cases such as dialogs instantiated by shinyjs appearing multiple times), but it also negates the need to set ignoreInit to TRUE to accommodate page switching. More importantly, it should also help with performance on certain applications with expensive computations within reactives.

I tested the new branch in a few scenarios and it seems to work well, but one side-effect I noticed is that the UI elements defined in shiny_ui are always rendered last. A modification of the previous example showcases the issue:

library(shiny)
library(shiny.router)

# This generates menu in user interface with links.
menu <- (
  tags$ul(
    tags$li(a(class = "item", href = route_link("home"), "Home page")),
    tags$li(a(class = "item", href = route_link("side"), "Side page"))
  )
)

# This creates UI for each page.
page <- function(title, content, button_name) {
  div(
    menu,
    titlePanel(title),
    p(content),
    actionButton("switch_page", "Click to switch page!"),
    actionButton(button_name, "Go!")
  )
}

# Both sample pages.
home_page <- page("Home page", uiOutput("current_page"), "test1")
side_page <- page("Side page", uiOutput("current_page"), "test2")

home_server <- function(input, output, session) {
  observeEvent(input$test1, {
    print("Foo")
  })
}

side_server <- function(input, output, session) {
  observeEvent(input$test2, {
    print("Bar")
  })
}

# Creates router. We provide routing path, a UI as
# well as a server-side callback for each page.
router <- make_router(
  route("home", ui = home_page, server = home_server),
  route("side", ui = side_page, server = side_server)
)

# Create output for our router in main UI of Shiny app.
ui <- shinyUI(fluidPage(
  actionButton("before_router", "Before router UI"),
  router_ui(),
  actionButton("after_router", "After router UI")
))

# Plug router into Shiny server.
server <- shinyServer(function(input, output, session) {
  router(input, output, session)

  output$current_page <- renderText({
    page <- get_page(session)
    sprintf("Welcome on %s page!", page)
  })

  observeEvent(input$switch_page, {
    if (is_page("home")) {
      change_page("side")
    } else if (is_page("side")) {
      change_page("home")
    }
  })
})

# Run server in a standard way.
shinyApp(ui, server)

Thanks for addressing the issue so fast @krystian8207! Being able to use shiny.router without modifications to the main application code (be it ignoreInit or assigning observers to variables in order to destroy them) would make the package incredibly versatile.

krystian8207 commented 4 years ago

@andbamp Thank you for noticing that. The current branch version should fix this behavior as well.