juba / robservable

Observable notebooks as R htmlwidgets
https://juba.github.io/robservable/
163 stars 11 forks source link

Shiny proxy #11

Closed timelyportfolio closed 3 years ago

timelyportfolio commented 4 years ago

Your recent changes to allow update without re-render are a very welcome addition. However, I wonder if it might be helpful for a user to send commands such as update through a proxy-like mechanism, so they don't have to renderRobservable({...}) every time they would like to do something with the instance. In my head, I envision something like robs_update(id, list(...)). This would allow a user to do something like the below while this isn't currently possible/convenient.

observeEvent( ... , {
  robs_update( 'myobs', list( data = ..., color = "red" )
})

or (example)

robsUpdate( proxy, list( data = ..., color = "red" ) )

If we only think update is something we might want then we can add with a couple of lines using what you have already done with redefine in your widget render.

If we think there are lots of future methods/functions then we can set up a formal proxy. In parcoords I borrowed heavily from the infrastructure of leaflet hoping to establish some standards around Shiny/JS proxy methods with full acknowledgement that this isn't perfect. Here are the applicable bits of code:

With methods, we get an added benefit for JS manipulation as demonstrated in https://timelyportfolio.github.io/parcoords/articles/introduction-to-parcoords-.html#snapshot.

I am happy to submit a pull but wanted to check with you to see if my thoughts might be aligned with yours. Please don't misinterpret my excitement for any attempt to take over :)

juba commented 4 years ago

I'm not familiar with the proxy design but it would definitely be a useful feature. It would be great if you find the time to implement it (I'll take time to take a deeper look at the code you referenced, which is very interesting).

Many thanks for the suggestion. And thanks for your excitement too, it is quite motivating :-)

timelyportfolio commented 4 years ago

@juba I added a prototype proxy mechanism in proxy branch. Currently, the proxy only supports methods for updating input variables. Please see the example.

I also think I found a bug in lines. Please take a look and confirm that this does not cause other problems.

juba commented 4 years ago

Thanks !

I don't think what you mention as a bug is really one (but I may be wrong). params.observers_variables is used to store the current list of set observers. When the widget is updated and not created from scratch, it is used to avoid to recreate an already existing observer by comparing params.observers and params.observers_variables if the latter exists.

If you thought this is a bug (and if it is not), then at the very least it means this is not well explained / well named.

I'll take a look at your proxy implementation shorty.

juba commented 4 years ago

Yup, it seems that the shiny_observers example doesn't work anymore in the proxy branch.

timelyportfolio commented 4 years ago

@juba I realized my "correction" does not work. I'll fix at some point today. Sorry.

timelyportfolio commented 4 years ago

@juba the issue/bug comes if this.params.observers_variables is undefined and won't have Object.keys. I think the probem is not relevant and only happened because I was trying to use the setter for params which did not work. The current implementation does not use the setter and I think I can just roll back my erroneous change.

timelyportfolio commented 4 years ago

@juba I rolled back the change and the observer and proxy examples both now work with no errors on the JavaScript side.

Questions

  1. because of async construction any proxy method sent prior to construction gets dropped. I am not sure how to prevent this besides set up a queue structure that will retain messages to deploy once construction is complete. I don't know at this point if it is worth the effort.
  2. what other methods might a user like/need? I could envision a user wanting to set an observer post render, so I gues we could add something to accomplish that. Anything else?
juba commented 4 years ago

Thanks for the rollback.

I get another issue with the proxy : when passing a data frame to the proxy update method, it is not converted by row, thus making the update fail. You should see this in the following example :

library(shiny)
library(robservable)

robs <- robservable(
  "@d3/bar-chart",
  include = "chart",
  input = list(color = "red", height = 700)
)

ui <- tagList(
  actionButton("btnChangeData", "Change Data"),
  robservableOutput("bar")
)
server <- function(input, output, session) {
  output$bar <- renderRobservable({
    robs
  })

  # set up a proxy to our bar robservable instance
  #   for later manipulation
  robs_proxy <- robservableProxy("bar")

  observeEvent(input$btnChangeData, {
    robs_update(
      robs_proxy,
      data = data.frame(
        name = LETTERS[1:10],
        value = round(runif(10)*100)
      )
    )
  })
}

shinyApp(ui, server)

In renderRobservable the following is added to make the data frame conversion compliant with the d3-array format. I tried to add it at various locations in your R proxy code, but to no avail :

attr(x, "TOJSON_ARGS") <- list(dataframe = "rows")

I keep your questions in mind !

juba commented 4 years ago

Regarding your questions :

  1. I'm not very familiar with proxy usage, but is it common to use the proxy outside a user input event or a timer ? Can the proxy be used to initialize the widget, or only to update it ? If the latter, I think, as you say, that dropping some proxy invocations may not be a problem.

  2. Again, as I'm not very familiar with proxy usage, I just took a look at the robservable arguments. Setting observers, as you say, may be useful. I don't think allowing to modify include and hide would be relevant. Do you think being able to change the width and height values could make sense ?

timelyportfolio commented 4 years ago

@juba I just pushed a crude queueing solution to handle methods sent prior to construction. This assumes that any proxy methods form the user are intended to override any conflicting instructions from initial render. I think this is a safe assumption.

height can be changed through update but I think you are right we will need an explicit method to change width. If we are adding an explicit method to change width, then I would think it best to also provide explicit method to change height even though the mechanism is slightly different.

I hadn't thought about include and hide. I like your idea.

I'll figure out how to handle data.frame.

timelyportfolio commented 4 years ago

@juba I think we can manually convert method arguments to json to handle the data.frame issue. See bf42409.

juba commented 4 years ago

Thanks for the handling of the data frame issue, it now works flawlessly. And the queuing solution is a nice addition !

Regarding width and height, widget sizing is a bit complicated, as it is an interaction between the width and (if it exists) height notebook values, and the width and height of the widget. So changing width and height with the update method would not be the same thing as changing the width and height argument of robservable (not sure if I'm totally clear here).

juba commented 4 years ago

I just added a small commit : when updating values, params.input should only contain the updated values. Otherwise, charts are redrawn instead of being animated with transitions.

timelyportfolio commented 4 years ago

@juba I added a proxy observe method in 8edda40. Here is an example.

library(shiny)
library(robservable)

robs <- robservable(
  "@d3/bar-chart",
  include = "chart",
  input = list(color = "red", height = 700)
)

ui <- tagList(
  robservableOutput("bar")
)
server <- function(input, output, session) {
  output$bar <- renderRobservable({
    robs
  })

  # set up a proxy to our bar robservable instance
  #   for later manipulation
  robs_proxy <- robservableProxy("bar")

  robs_observe(robs_proxy, "color")

  observeEvent(input$bar_color, {
    print(input$bar_color)
  })

  observe({
    invalidateLater(2000, session)

    # update with random color
    robs_update(
      robs_proxy,
      color = paste0(
        "rgb(",
        paste0(col2rgb(colors()[floor(runif(1,1,length(colors())))]),collapse=","),
        ")"
      )
    )
  })
}

shinyApp(ui, server)
juba commented 4 years ago

Very nice !

What I really like is that both the proxy and the "classic" approaches can be used, so the user really has a choice here depending on its code and its habits.

timelyportfolio commented 4 years ago

@juba agree and is a testament to your architecture of the widget.

timelyportfolio commented 4 years ago

@juba I do not think include proxy will be easy to accomplish. I think the steps would need to be the following, but I will need to do some experiments:

  1. create output div
  2. new Inspector(output div created in step1)
  3. inspector from step2 function(variable)
juba commented 4 years ago

If it is not straightforward, maybe we should wait to see if there is a use case for this ?

I don't really see one for the moment...

timelyportfolio commented 4 years ago

@juba I think I have it figured out, but we will run into other problems, such as ordering of cells and potential name conflicts. I'll add the code here so we can refer to it later, but I might pass on adding it at this moment.

// create new div container
const div = create_output_div(name)
// add new div to the widget dom element
el.appendChild(div)
// define new variable that depends on name variable
//   we will need to construct a unique name to avoid any conflicts
el.module.main.variable(observablehq.Inspector.into(div))
  .define(unique_name, [name])
juba commented 4 years ago

I agree with you, I don't think it would be worth the complexity and the potential issues it could cause. Having a proxy mechanism for update and observe should cover the most useful cases, by far.

Do you think we could merge the proxy branch into master ?

timelyportfolio commented 4 years ago

@juba pull #32 submitted. Hope this is ok. I would like (hope I have time) to write a short vignette.

juba commented 4 years ago

I just reorganised the shiny proxy sample apps in the examples directory, and added links to them in your proxy section in the shiny vignette. Hope it's ok for you.

timelyportfolio commented 4 years ago

@juba absolutely ok and thanks. I hesitated on how you wanted me to integrate.

juba commented 3 years ago

Closing this for now. Feel free to reopen if needed !