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

Event_data in 4.9.0 missing code to process situation if data is NULL #1538

Closed
ghost opened this issue May 23, 2019 · 12 comments
Closed

Event_data in 4.9.0 missing code to process situation if data is NULL #1538

ghost opened this issue May 23, 2019 · 12 comments

Comments

@ghost
Copy link

ghost commented May 23, 2019

It seems that in the new R plotly 4.9.0 version, there is a change in the event_data code that causes errors if the plot is not yet build.
i.e. a situation where a plot_ly plot is not build because the data it needs is not yet available in the plot by means of a req(data) inside the plot_ly() block. The observeEvent(event_data("plotly_click", source = 'A_plot'), { ....}) used to work fine before, but now throws errors, and does so for all forms of event_data.

example of an error message I get now:

Warning: The 'plotly_click' event tied a source ID of 'plotlyplot.testplot' is not registered. In order to obtain this event data, please add event_register(p, 'plotly_click') to the plot (p) that you wish to obtain event data from.

The app below demonstrates the problem with version 4.9.0:

   library(shiny)
    library(plotly)



rm(list = ls(), envir = environment()) ## to prevent cross over from old runs

testData = data.frame(day = sample(seq(as.Date('1999/01/01'), as.Date('2000/01/01'), by="day"), 24), frequency = sample(1:5, 24, replace = T ), datecoloring = sample(1:2, 24, replace = T ))
testData$dayPOSIXct <- as.POSIXct(testData$day)

dateRangeMin <- min(testData$day)
dateRangeMax <- max(testData$day)

ui <- fluidPage(
  actionButton(inputId = 'Load', label = 'Data'),
  plotlyOutput('testplot', width = 700, height = 500)

)


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

  values <- reactiveValues()

  observeEvent(input$Load, { 
    values$testData <- testData
    })

  output$testplot <- renderPlotly({ 
    req(values$testData)
    p <-  plot_ly(data = values$testData, source = 'testplot',
                  color  = as.factor(values$testData$datecoloring), colors = c('#339fff', '#eaf5ff'),
                  opacity= 0.5, showlegend = T,
                  marker = list(line = list(width = 2, color = '#0000ff')),
                  hoverinfo = "text",
                  text = ~paste('Files:', values$testData$frequency, '<br>Date:', format(values$testData$day, format = '%Y-%m-%d'), sep = ' '))%>%
      add_bars( x = ~dayPOSIXct, y =  ~frequency,   type = "bar", width = 36000000
      )
    p
  })

  observeEvent(event_data("plotly_relayout", source = "testplot"),{
    #any code here, doesn't matter, bug happens already
  })


}

shinyApp(ui, server)
@bklingen
Copy link

related to issue #1528

@cpsievert
Copy link
Collaborator

It's important to note that this is a warning, not an error. In this case, the warning is a false positive...notice that this does right thing (prints the relayout event to the R console)

observeEvent(event_data("plotly_relayout", source = "testplot"),{
    print(event_data("plotly_relayout", source = "testplot"))
  })

It's unfortunate, but I don't think I can do anything to suppress the warning in this sort of example (i.e. using observeEvent()), so I'll be closing here, but #1528 seems to be a slightly different issue that I might be able to do something about

@ghost
Copy link
Author

ghost commented May 23, 2019

It is still highly annoying to have false positive warnings. Also since suppressMessages() and suppressWarnings() wrappers don't seem to affect it at all. I'm trying to write my app with zero console printing and 9 pages of plotly errors doesn't look very professional. version 4.8.0 didn't have the warning and I don't see why we need it to be honest.

@cpsievert
Copy link
Collaborator

cpsievert commented May 23, 2019

I understand it's annoying, but there are good technical reasons as to why event_data() now throws a warning in this case. On page load, you're calling event_data() and referencing a plot that doesn't yet exist, and there's no way for event_data() to know that the plot might exist in the future, so it throws a warning.

If you need to get rid of the warnings, you can do this

relayout_data <- reactive({
    req(values$testData)
    event_data("plotly_relayout", source = "testplot")
  })
  
  observeEvent(relayout_data(),{
    print(relayout_data())
  })

The req() here will make sure you're not calling event_data() before the relevant plotly graph is rendered.

@ghost
Copy link
Author

ghost commented May 23, 2019

Thanks for the help cpsievert! Don't ask me how, but this still gives me the error messages. I'm going to try and find out if I can reproduce it in a minimal example.

Ok, small update: the plot I'm trying to solve this for is normally inside a dropdownButton. Once I moved the plot out into the main page, the warning disappeared. However, if the plot is somewhere else, the reactive approach still happily generates the error, as the data needed for the plot is available (after going through the loading files procedure in my app, but the plot is not build until the dropdownbutton is opened, and between loading data and displaying the plot, the warning still happens..... sadly

I don't see how there is a way to construct a way to so to say "req("is the plot actually build yet")" approach

@ghost
Copy link
Author

ghost commented May 23, 2019

I would be very curious to hear the necessity for the warning message and why returning NULL until there is actual event data isn't sufficient.
I literally tried every trick in the book (that I know) to suppress messages, warnings, trycatch, !is.null, req etc etc, but while some do stop further code, the warning message is printed.

I can understand perhaps some need for a specific output in some situations, but I honestly don't see the need for merely printing a message on console that you can't get rid off no matter what. It's just a message, without any further implications on anything so I don't see how that "is needed" as you stated.

@ismirsehregal
Copy link

ismirsehregal commented May 24, 2019

For future readers: This is the related SO question. In the end it's still possible to prevent the warning using req() (also with the dropdownButton - see this).

@RyanMorton-USDA
Copy link

RyanMorton-USDA commented Jul 23, 2024

Adding the req() creates a new dependency for that reactive object that causes other problems. If the values are dependent on another screen, the req() will pull the user to the downstream use of the event_data, like drill-down on another tab. This is not a safe solution for many of us - particularly when using priority = 'event'.

@cpsievert
Copy link
Collaborator

@RyanMorton-USDA a fix was recently merged related to this (#2337). Do your problems go away with the development version (remotes::install_github("plotly/plotly.R"))?

@RyanMorton-USDA
Copy link

@RyanMorton-USDA a fix was recently merged related to this (#2337). Do your problems go away with the development version (remotes::install_github("plotly/plotly.R"))?

First, thank you! We can give it a go, but it's a largish gov't app that needs stable versions. If it does work, any idea on a release date.

@asadow
Copy link

asadow commented Oct 16, 2024

@RyanMorton-USDA
How about tracking rendering using a reactiveVal?

## Reactive for tracking if the plot has been rendered
plot_rendered <- reactiveVal()

output$testplot <- renderPlotly({ 
    req(values$testData)
    plot_rendered(FALSE)
    p <-  plot_ly()
    session$onFlushed(\() plot_rendered(TRUE), once = FALSE)
    p
)}

then

relayout_data <- reactive({
    req(plot_rendered())
    event_data("plotly_relayout", source = "testplot")
  })

@dvg-p4
Copy link
Contributor

dvg-p4 commented Dec 12, 2024

@asadow

How about tracking rendering using a reactiveVal?

    session$onFlushed(\() plot_rendered(TRUE), once = FALSE)

I'm doubtful that this elaborate workaround will actually solve the problem in a larger app, mainly since the code that actually throws this warning is already waiting on a session flush:

plotly.R/R/shiny.R

Lines 151 to 156 in cc49ee5

session$onFlushed(
function() {
eventIDRegistered <- eventID %in% session$userData$plotlyShinyEventIDs
if (!eventIDRegistered) {
warning(
"The '", event, "' event tied a source ID of '", source, "' ",

But even if it did suppress the warning, I'd still feel that doing such an elaborate workaround shouldn't be necessary. There's a performance penalty, and even more importantly a code complexity penalty, to adding additional reactive values. Whereas it would be trivial for plotly to add a way to suppress it that didn't add such additional complexity.

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

6 participants