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

[Feature Request] - Support reactives in addition to reactiveValues #162

Closed
kieran-mace opened this issue May 14, 2021 · 6 comments
Closed

Comments

@kieran-mace
Copy link

In addition to this functionality, that uses reactiveValues and observeEvent:

  data_rv <- reactiveValues(data = iris, name = "iris")
  
  observeEvent(input$data, {
    if (input$data == "iris") {
      data_rv$data <- iris
      data_rv$name <- "iris"
    } else {
      data_rv$data <- mtcars
      data_rv$name <- "mtcars"
    }
  })
  
  esquisse_server(
    id = "esquisse", 
    data_rv = data_rv
  )

also support the use of reactives:

  data_reactive <- reactive({
    if (input$data == "iris") {
      data <- iris
    } else {
      data <- mtcars
    }
    return(data)
  })
  
  esquisse_server(
    id = "esquisse", 
    data_rv = data_reactive()
  )

according to the developer of shiny reactive is far preferred over the combination of reactiveValue + observeEvent

@kieran-mace
Copy link
Author

reactive is for calculating values without side effects
observe is for producing side effects

@pvictor
Copy link
Member

pvictor commented May 15, 2021

Yes might be possible to support a reactive function there. But with your example esquisse_server won't have the name of the dataset to generate the code.

Currently reactiveValue does what I need, that's sufficient for me.

Victor

@teofiln
Copy link

teofiln commented May 19, 2021

I had a similar issue. One way to circumvent this is to resolve the reactive and assign the dataset to the global environment with <<-.

  observeEvent(my_reactive_data(), {
    req(my_reactive_data())
    esq_data <<- my_reactive_data() # assign to global env
    data_r$data <- esq_data
    data_r$name <- "esq_data"
  })
  
  results <- esquisse_server(
    id = "esquisse",
    data_rv = data_r
  )

Not sure if its safe or preferred. I'd be interested to see alternative solutions.

HTH!

@daattali
Copy link
Contributor

daattali commented Nov 4, 2022

It's true that the current implementation using reactiveValues works, but I do also think that it's awkward/not intuitive. It would be more clear and more in-line with most of the other shiny extensions if instead there would be two separate variables "data" and "name" and both of them would take a reactive. This will also organically solve another issue: using one parameter to group together two parameters is also not a great practice. It will also solve #227

I'm usually extremely conservative with breaking API changes, but I feel strongly enough about this that I would even argue it's worth changing the data argument so that it only expects a dataframe (either static or reactive) and adding a name argument (that accepts a string or a reactive string). To help users that have old code that uses reactiveValues, the data argument could be checked to see if it contains an object of class reactiveValues and if it does then give a very informative error message on how to update the code.

@daattali
Copy link
Contributor

daattali commented Nov 4, 2022

An example of the difference it would make in code:

Current:

ui <- fluidPage(
  selectInput("dataset", "dataset", c("iris", "mtcars")),
  esquisse::esquisse_ui("test")
)

server <- function(input, output, session) {
  rv <- reactiveValues(data = data.frame(), name = "")
  observeEvent(input$dataset, {
    rv$data <- get(input$dataset)
    rv$name <- input$dataset
  })
  esquisse::esquisse_server("test", rv)
}

shinyApp(ui, server)

Proposed:

ui <- fluidPage(
  selectInput("dataset", "dataset", c("iris", "mtcars")),
  esquisse::esquisse_ui("test")
)

server <- function(input, output, session) {
  data <- reactive({
    get(input$dataset)
  })
  name <- reactive({
    input$data
  })
  esquisse::esquisse_server("test", data = data, name = name)
}

shinyApp(ui, server)

And it would also be nice to be able to do without reactive values:

ui <- fluidPage(
  esquisse::esquisse_ui("test")
)

server <- function(input, output, session) {
  esquisse::esquisse_server("test", data = iris, name = "iris")
}

shinyApp(ui, server)

@pvictor
Copy link
Member

pvictor commented Nov 24, 2022

Thanks to all for suggesting this evolution and to @kieran-mace for initiating it.
Now you can use of these three methods to pass data to module :

library(esquisse)
library(shiny)

ui <- fluidPage(
  esquisse_ui("test")
)

server <- function(input, output, session) {
  
  ## with reactivalues
  # rv <- reactiveValues(data = iris, name = "iris")
  # esquisse_server("test", rv, import_from = NULL)
  

  ## with reactive()
  esquisse_server("test", reactive(iris), name = reactive("iris"), import_from = NULL)
  
  ## with data.frame
  # esquisse_server("test", iris, name = "iris")
}

shinyApp(ui, server)

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

No branches or pull requests

4 participants