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

[Bug]: Document the different behaviour of active tab and hidden tab when using Data module #1303

Closed
3 tasks done
vedhav opened this issue Aug 8, 2024 · 6 comments · Fixed by #1373
Closed
3 tasks done
Assignees
Labels

Comments

@vedhav
Copy link
Contributor

vedhav commented Aug 8, 2024

What happened?

There is a different behaviour of data flow in modules when using transform during init.
The transform module server seems to be called for every module. But, shiny can trigger reactive values differently based on the shown/hidden state of the module. This can causing different behaviour for the active tab and hidden tab. It would be best to showcase this in the vignette about teal data module creation.

In the following example you can see that we have two teal_modules that are identical. However, we get a validation error in second module which is hidden and the first module works fine.

devtools::load_all("teal")

my_transformers <- list(
  teal_transform_module(
    label = "Keep first 6 from IRIS",
    ui = function(id) {
      ns <- NS(id)
      div(
        checkboxInput(ns("check"), label = "Toggle `head(iris)`"),
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        eventReactive(input$check, {
          print("Check triggered")
          req(data())
          if (input$check) {
            within(data(), iris <- head(iris, 6))
          } else {
            data()
          }
        })
      })
    }
  )
)

data <- teal_data(iris = iris)
app <- init(
  data = data,
  modules = modules(
    example_module("Module with transformations 1", transformers = my_transformers),
    example_module("Module with transformations 2", transformers = my_transformers)
  )
)

shinyApp(app$ui, app$server)

sessionInfo()

No response

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct.

Contribution Guidelines

  • I agree to follow this project's Contribution Guidelines.

Security Policy

  • I agree to follow this project's Security Policy.
@vedhav vedhav added bug Something isn't working core labels Aug 8, 2024
@gogonzo gogonzo added this to the teal transform milestone Aug 8, 2024
@gogonzo gogonzo self-assigned this Aug 9, 2024
@averissimo averissimo mentioned this issue Aug 9, 2024
63 tasks
@averissimo averissimo self-assigned this Aug 9, 2024
@averissimo
Copy link
Contributor

@vedhav can you confirm that this was fixed in #1253 ?

@vedhav
Copy link
Contributor Author

vedhav commented Aug 13, 2024

This issue (a slightly different one) persists. When I run the app mentioned in this issue I get the error message in the second tab that could have been avoided.

Screenshot 2024-08-13 at 2 56 27 PM Screenshot 2024-08-13 at 2 56 33 PM

@averissimo
Copy link
Contributor

TL;DR: This is a consequence of our transform design that penalizes transform modules that are not reactive to data()

Action: Do you think we can detect or mitigate it?

Context:

What happens here is that the data is changing, but the eventReactive is not sensitive to that change.

The app developer should either:

  1. use eventReactive(list(input$check, data()), { ... }
    • note the data() in the eventExpr argument
  2. or totally avoid eventReactive(...)

In the example below you can see that in the first tab the first transformers don't affect the result as it's only reactive to input$check

Screencast.from.2024-08-13.15-17-20.webm
options(
  teal.log_level = "DEBUG",
  teal.show_js_log = TRUE,
  # teal.bs_theme = bslib::bs_theme(version = 5),
  shiny.bookmarkStore = "server"
)

devtools::load_all("teal")


my_transformers <- list(
  teal_transform_module(
    label = "Keep first 10 from IRIS",
    ui = function(id) {
      ns <- NS(id)
      div(
        checkboxInput(ns("check"), label = "Toggle keep only first 10 rows?"),
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        result <- reactive({
          print(glue::glue("Check triggered {session$ns('id')} { input$check %||% '(null)' }"))
          req(data())
          if (input$check) {
            within(data(), iris <- head(iris, 10))
          } else {
            data()
          }
        })
        
        result
      })
    }
  ),
  teal_transform_module(
    label = "Sepal.length negative?",
    ui = function(id) {
      ns <- NS(id)
      div(
        checkboxInput(ns("check"), label = "Negative Sepal.length?"),
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        result <- reactive({
          print(glue::glue("Check triggered {session$ns('id')} { input$check %||% '(null)' }"))
          req(data())
          if (input$check) {
            within(data(), iris$Sepal.Length <- iris$Sepal.Length * -1)
          } else {
            data()
          }
        })
        
        result
      })
    }
  ),
  teal_transform_module(
    label = "Keep first 6 from IRIS",
    ui = function(id) {
      ns <- NS(id)
      div(
        checkboxInput(ns("check"), label = "Toggle `head(iris)`"),
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        eventReactive(input$check, {
          print(glue::glue("Check triggered {session$ns('id')} { input$check %||% '(null)' }"))
          req(data())
          if (input$check) {
            within(data(), iris <- head(iris, 6))
          } else {
            data()
          }
        })
      })
    }
  )
)

my_transformers_good <- append(
  my_transformers[1:2],
  list(
    teal_transform_module(
      label = "Keep first 6 from IRIS (good)",
      ui = function(id) {
        ns <- NS(id)
        div(
          checkboxInput(ns("check"), label = "Toggle `head(iris)`"),
        )
      },
      server = function(id, data) {
        moduleServer(id, function(input, output, session) {
          eventReactive(list(input$check, data()), {
            print(glue::glue("Check triggered {session$ns('id')} { input$check %||% '(null)' }"))
            req(data())
            if (input$check) {
              within(data(), iris <- head(iris, 6))
            } else {
              data()
            }
          })
        })
      }
    )
  )
)

data <- teal_data(iris = iris)
app <- init(
  data = data,
  modules = modules(
    example_module("Module with transformations 1", transformers = my_transformers),
    example_module("Module with transformations 2", transformers = my_transformers_good)
  )
)

shinyApp(app$ui, app$server)

@vedhav
Copy link
Contributor Author

vedhav commented Aug 29, 2024

Adding the general conclusion we had related to this issue.
In general, shiny behaves this way. When the UI is hidden some reactive values are not triggered and I don't think we would be able to initialize the values for some cases. So, it is best to recommend that the teal_data_module author to pass the teal_data object as reactive even when the module is hidden.
We should change the scope of this issue to: Add this behaviour in the vignette of the teal_data_module construction.

@vedhav vedhav changed the title [Bug]: There is a different behaviour of data flow in modules when using transform during init [Bug]: Document the different behaviour of active tab and hidden tab when using Data module Aug 29, 2024
@m7pr m7pr removed the bug Something isn't working label Aug 30, 2024
@gogonzo
Copy link
Contributor

gogonzo commented Sep 17, 2024

I have a feeling that something is missing. If at least one module (first one) performs properly, then maybe we can do something to enable the same behaviour to the second one. Like:

  • to postpone to trigger srv_teal_module until the module is_active (module can't be active if data is not loaded, because tabs are blocked)

@gogonzo
Copy link
Contributor

gogonzo commented Oct 7, 2024

@donyunardi PR is coming to resolve the root cause. @averissimo is taking care of it 💪

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

Successfully merging a pull request may close this issue.

5 participants