Added skipFirst arg to observeEvent #1494

bborgesr · 2016-12-05T13:07:50Z

As we discussed (prompted by #1486), this adds a new argument (skipFirst) to observeEvent and eventReactive that, when TRUE, will cause the action not to be triggered the very first time that the observer/reactive is run.

This allows for, for example, observers for dynamically created action buttons not to be triggered when the button is created, and only when it is actually clicked.

Consider the following app:

library(shiny)

ui <- fluidPage(
  textInput("divID", "Enter div ID:", ""),
  actionButton("isrt", "Create My Button"), 
  tags$div(id = "placeholder")
)

server <- function(input, output, session) {
  rv <- reactiveValues(btnExists = FALSE)
  
  # take a dependency on `isrt` button
  observeEvent(input$isrt, {
    
    # handle the case when user does not provide ID
    divID <- if (input$divID == "") "id" else input$divID
    btnID <- paste0(divID, "rmv")
    
    # only create button if there is none
    if (!rv$btnExists) {
      
      insertUI(
        selector = "#placeholder",
        ui = tags$div(id = divID, 
                      actionButton(btnID, "Remove this button"))
      )
      rv$btnExists <- TRUE
      print("created")
      
      # create a listener on the newly-created button that will
      # remove it from the app when clicked
      observeEvent(input[[btnID]], {
        removeUI(selector = paste0("#", divID))
        rv$btnExists <- FALSE
        print("destroyed")
        
      }, skipFirst = TRUE)
      
      # otherwise, print a message to the console
    } else {
      message("The button has already been created!")
    }
  })
}

shinyApp(ui = ui, server = server)

This PR makes this app (that was completely broken before) work as it should, at least superficially: i.e. every time you click "Create my button", it will do so if there isn't a button there already. And each time you click on the newly-created button, it will remove itself. You can create and remove the button with the same id as many times as desired, and you can also change the id whenever you want.

BUT...

Unfortunately, that's not the end of the story. If you look closely at the app above, you'll see that it prints to the R console the word created whenever a button is created and destroyed whenever a button is destroyed. The good news is that every time a button is created or destroyed, the messages print, and that every time that one of the buttons (or the insert or remove button) is clicked, the appropriate message prints. This is why it appears from a user perspective that the app is working just fine. The bad news is that the destroyed message will be displayed x number of times, with x being the number of times that a button with that ID has been created before. So if you toggle between the "Create" and "Remove" buttons, from app startup, your console will look like:

> runApp('insertUIremoveUI.R')

Listening on http://127.0.0.1:6086
[1] "created"
[1] "destroyed"
[1] "created"
[1] "destroyed"
[1] "destroyed"
[1] "created"
[1] "destroyed"
[1] "destroyed"
[1] "destroyed"
[1] "created"
[1] "destroyed"
[1] "destroyed"
[1] "destroyed"
[1] "destroyed"
[1] "created"
[1] "destroyed"
[1] "destroyed"
[1] "destroyed"
[1] "destroyed"
[1] "destroyed"

When really, it should be this:

> runApp('insertUIremoveUI.R')

Listening on http://127.0.0.1:6086
[1] "created"
[1] "destroyed"
[1] "created"
[1] "destroyed"
[1] "created"
[1] "destroyed"
[1] "created"
[1] "destroyed"
[1] "created"
[1] "destroyed"

I.e. if a button has already been destroyed, any observers that had been set up for it should disappear. This is, of course, possible, if we change the appropriate code to:

     obs <-  observeEvent(input[[btnID]], {
        removeUI(selector = paste0("#", divID))
        rv$btnExists <- FALSE
        print("destroyed")
        obs$destroy()
        
      }, skipFirst = TRUE)

However, this should either 1) have a user-friendly wrapper / became (yet) another argument or 2) be a side effect of removeUI() -- the destruction of all observers or reactives that depend on that input.
Is 2) really such a terrible idea? (I can see how this would get tricky if we're talking about an observe instead of an observeEvent -- or even an observeEvent that has something more complicated for the event than input[[btnID]])... But, it really feels like there should be an easier way of (at least optionally) getting rid of an observeEvent when the input it is responding to is removed...

Thoughts?

cc @jcheng5 @wch

wch · 2016-12-05T15:41:45Z

IMO, having the user call obs$destroy() isn't too much to ask when they're doing stuff like this.

I think that (2) definitely isn't the way to go, because it's too magical. insertUI and removeUI are pretty low-level commands.

wch · 2016-12-07T17:16:48Z

One danger of using getCurrentObserver is that it is accessible from outside the observer proper. If it's called from within a reactive, it also returns the current observer, but that may be undesirable.

For example, here's how accessing the current observer from a reactive can get you in trouble, because the value of the reactive is cached:

r <- reactive({
  getCurrentObserver()$.label
})

observe(label = "observer 1", {
  print(r())
})

observe(label = "observer 2", {
  print(r())
})

shiny:::flushReact()
# [1] "observer 1"
# [1] "observer 1"

wch · 2016-12-07T17:55:49Z

After some discussion with Barbara, here are two things that we thought might be useful:

Make it so getCurrentObserver() can only be called from inside the observe() or observeEvent(), or regular functions that they invoke -- but not from any reactives that they call.
Add an argument like runOnce to observe() and observeEvent(), which would tell an observer to just run once and then destroy itself. This would sort of be like the $.one() function in jQuery, which I find handy.

jcheng5 · 2016-12-07T21:25:28Z

I just realized that skipFirst is going to have an issue with bookmarkable state. Or does observeEvent already have an issue with bookmarkable state?

jcheng5 · 2016-12-07T21:27:02Z

I was thinking the same for getCurrentObserver().

runOnce (or just once) would definitely be useful for observeEvent. Would it also be useful for observe?

bborgesr · 2016-12-08T01:24:36Z

@jcheng5 re once: IMO the case for having this for both observeEvent and observe is that the latter is equal (and superior, right?) in power to the former (in the sense that you can always do the job of an observeEvent by using observe + isolate). So, I think that anything that we add to observeEvent, we can also add to observe, keeping them lined up in that way. Intuitively, that seems more consistent to me, but maybe I'm looking at this from a weird perspective (considering what is theoretically possible instead of what is actually best suited for real use cases).

jcheng5 · 2016-12-08T02:38:00Z

@bborgesr observeEvent breaks it into two parts. "once" I'm assuming means the second part (handlerExpr) runs once. So you're saying "when this executes, whenever that may be, then destroy." For observe, that is always going to happen at the end of the current flushReact (unless you start it suspended) so "once" really means "execute this code after the current tick", in which case observe(once=TRUE) is a really weird way to express that.

I don't think I'm explaining well, but do you get the gist of it?

bborgesr · 2016-12-08T17:02:20Z

@jcheng5, I think I get it: since the eventExpr is (possibly) woven throughout the body of the observe call, we can't know beforehand when all of the reactive dependencies are "ready" (since unlike observeEvent, the concept of being "ready", doesn't really apply to observe). So, I think I get it. Basically, the reason why it's doesn't make much sense to have once for observe falls along the same lines we also don't have ignoreNULL for observe.

Another question: do you think once makes sense for eventReactive? From the conversation I had with Winston, we agreed that it wouldn't make sense because eventReactive should never be used for side-effects (i.e. if you need to do both side effect and value calculation, you should do that in observeEvent and save the calculated value in a reactiveValues slot). For example, using the same app as before, imagine that we want to, in the inner observeEvent(), both remove the UI and return the ID of what was removed, so we can display that to the user. We could continue using observeEvent like so:

rv <- reactiveValues(btnExists = FALSE, justRemoved = NULL)
...
      observeEvent(input[[btnID]], {
        removeUI(selector = paste0("#", divID))
        rv$btnExists <- FALSE
        rv$justRemoved <- divID
      }, skipFirst = TRUE, once = TRUE)
...
output$txt <- renderText({ paste("You just removed:", rv$justRemoved) })

Or, if we wanted to, it would be possible to adapt this into an eventReactive, but I think it mixes up imperative logic and reactive logic too much to be worth it:

justRemoved <- function() NULL
...
      justRemoved <<-  eventReactive(input[[btnID]], {
        removeUI(selector = paste0("#", divID))
        rv$btnExists <- FALSE
        return(divID)
      }, skipFirst = TRUE, once = TRUE)
...
output$txt <- renderText({ paste("You just removed:", justRemoved()) })

In this case, the once = TRUE would mean that the first time justRemoved() is invoked, it would also destroy itself (which is another whole can of worms because reactives don't have an exiting destroy method at this point). So that if you call justRemoved() a second time you would get an error. I really dislike this whole idea, but it thought it was worth it to share with you on the off chance that you actually can make any sense of this...

jcheng5 · 2016-12-08T17:12:21Z

eventReactive once doesn't make sense to me. If anything I think if you did that you'd want subsequent calls to the eventReactive to return the last cached value but never invalidate. But yeah doesn't seem like a good idea to pursue without a clear scenario.

I do think we should resolve the bookmarking issue before deciding to merge this though. @wch any thoughts?

bborgesr · 2016-12-08T17:26:35Z

re: bookmarking -- so if once = TRUE, then the observer is destroyed (which means that its private var .destroyed is set to TRUE). Doesn't that information (each observer and it's variables and methods) get stored in the state when a user clicks "bookmark"?

bborgesr · 2016-12-08T17:44:46Z

oh wait, you were talking about bookmarking + skipOnce... yes, that looks like it would be an issue. Can we solve it the same way? I.e. set a private var in observer called .initializedsuch that:

if skipFirst = FALSE (default), then .initialized = TRUE and never changes;
if skipFirst = TRUE, then .initialized = FALSE at the start and gets set to TRUE the first time its run method is executed (and never changes again after that).

For bookmarking to work with this, we just need the run method to change from:

run = function() {
      ctx <- .createContext()
      .execCount <<- .execCount + 1L
      ctx$run(.func)
    }

to:

run = function() {
   if (.initiliazed) {
      ctx <- .createContext()
      .execCount <<- .execCount + 1L
      ctx$run(.func)
   }
}

What do you think?

jcheng5 · 2016-12-08T19:38:21Z

Talked to @wch and we don't see a problem with bookmarkable state. But we both agreed we should think a little more about alternatives to the param name skipFirst.

wch · 2016-12-12T15:37:35Z

R/reactives.R

+#'   \item \code{observe({              getCurrentObserver()      }) }
+#'   \item \code{observe({ (function(){ getCurrentObserver() })() )} }
+#'   \item \code{observe({ isolate({    getCurrentObserver() })   }) }
+#'   \item \code{observe({ reactive({   getCurrentObserver() })   }) }


I think this will be confusing for most people. This is a bit clearer:

r <- reactive(getCurrentObserver()); observe(r())

wch · 2016-12-12T15:39:33Z

R/reactives.R

+#'   \item \code{observe({ reactive({   getCurrentObserver() })   }) }
+#' }
+#'
+#' In (1), since you're outside of a reactive context, we've already


I'd change this to simply, "In (1), since it is outside of a reactive context, \code{getCurrentObserver()} will return \code{NULL}."

wch · 2016-12-12T15:46:43Z

R/reactives.R

+#' it will always return \code{NULL}. There are a few subtleties, however.
+#' Consider the following five situations:
+#'
+#' \enumerate{


I think it would be useful to show, after each item, what gets returned, so that the reader doesn't have to scroll down to find out what the result is.

wch · 2016-12-12T15:47:42Z

R/reactives.R

+#' observer if we're inside an inner function's scope, or inside of an
+#' \code{isolate} or a \code{reactive} block?
+#'
+#' Before we can even asnwer that, there is an important distinction to


Typo: "asnwer". Although... I think perhaps this paragraph isn't necessary in these function docs. Maybe just something like this:

The expressions in observe(), reactive(), and isolate() create a new reactive context when they are executed. In contrast, regular function calls do not create new reactive contexts; they are not aware of reactivity.

Normally, getCurrentObserver() looks only at the current reactive context and, if it was created by an observer, it returns that observer. If getCurrentObserver() is called from another reactive context -- for example, one created by invoking a reactive() or isolate() -- then, because the context was not created by an observer, getCurrentObserver() will simply return NULL. If an observer calls a regular function that calls getCurrentObserver(), then it returns the observer, because the regular function has not created a new reactive context.

wch · 2016-12-12T15:50:45Z

R/reactives.R

+#'     observeEvent(input$go, {
+#'       print(paste("This will only be printed once; all",
+#'             "subsequent button clicks won't do anything"))
+#'       getCurrentObserver()$destroy()


I think this particular example isn't a good use of getCurrentObserver(), because the new once option is more appropriate for this case.

wch · 2016-12-12T18:00:41Z

R/reactives.R

+#'   \item \code{NULL}
+#' }
+#'
+#' Now, you may be wondering why \code{getCurrentObserver()} should't be able


It is also possible to look for the most recent observer in the reactive context "stack" by using dig=TRUE. The reactive context "stack" is analogous to a function call stack, except it tracks reactive contexts instead of function calls. With this option, when getCurrentObserver() is called from a reactive or isolate, it will return the observer at the bottom of the stack. (Because observers are never called by other observers, there can only be one observer in the stack.)

wch · 2016-12-12T18:02:17Z

R/reactives.R

@@ -1583,6 +1708,20 @@ maskReactiveContext <- function(expr) {
 #' @param ignoreNULL Whether the action should be triggered (or value
 #'   calculated, in the case of \code{eventReactive}) when the input is
 #'   \code{NULL}. See Details.
+#' @param skipFirst Whether the action should be run the first time the


When there's this much information, I'd suggest moving it outside of the documentation for the argument, to the main documentation section.

It's not the first time it's invalidated. It's when it's first created. How about ignoreInitial? "When TRUE, the handler never runs when the observer is first created (even if the event expression returns a non-NULL value)."

I like ignoreInitial, especially because of keeping the same vocab as ignoreNULL. But I feel like ignoreFirst rolls off the tongue better. Is there a particular reason you're leaning towards "initial" instead of "first", @jcheng5?

Sorry I actually should have said ignoreInit, not initial. First and initial both sound like they could be "the first time the handler would run, skip/ignore it". But it really means "when the observer is first created/initialized, skip/ignore the handler whether it is otherwise supposed to run or not".

Ah, perfect! Thanks @jcheng5! That is definitely the clearest way to explain this that any of us has come up with so far! This: "when the observer is first created/initialized, skip/ignore the handler whether it is otherwise supposed to run or not" makes the purpose of ignoreInit so much clearer even in my own head!

bborgesr · 2016-12-13T15:27:34Z

@wch, @jcheng5: how are you feeling about param name dig? It's admittedly very weird-sounding, but it has the advantage of not being overloaded... At it does make sense if you read the documentation. On Friday, @wch suggested search which is a lot more sane-sounding, but is of course overloaded... Any other thoughts?

jcheng5 · 2016-12-13T15:37:55Z

I don't hate dig but call stacks are typically described as growing downward, not upwards. I don't know why but it's always been like this afaik.

bborgesr · 2016-12-13T15:47:55Z

@jcheng5: how about bubble then? In the docs, it would read something like:

If bubble = TRUE, then the function will "bubble up" through the reactive contexts until it retrieves the one for the observer and returns the observer.

This makes more sense, if we like the dig metaphor, but we also acknowledge that stacks grow downward, not upward. On the downside, it's an argument name bordering on outright ridiculousness...

jcheng5 · 2016-12-13T15:49:18Z

How about recurse or recursive or inherit?

bborgesr · 2016-12-13T15:50:27Z

I like recursive

…or `getCurrentObserver`

bborgesr · 2016-12-19T23:35:39Z

As of now, this PR should be ready to merge. We went through lots of iterations but ended up agreeing on the following:

add arg once to observeEvent that causes it to self-destruct after the first time it's run
add arg ignoreInit to both observeEvent and eventReactive that causes them to not run when they are created/initialized

For now, we've agreed to table getCurrentObserver() for another time (the progress made is currently stored in a tag). This means that, for now, people will have to continue to do obs <- observeEvent( .. ; obs$destroy()) if they want to destroy the observer on some particular condition. However, the hope is that the once arg will cover 90% of these cases (for an example, see the app below).

For possible future reference, here's the app that started this conversation with the correct working code:

library(shiny)

ui <- fluidPage(
  textInput("divID", "Enter div ID:", ""),
  actionButton("isrt", "Create My Button"), 
  tags$div(id = "placeholder")
)

server <- function(input, output, session) {
  rv <- reactiveValues(btnExists = FALSE)
  
  # take a dependency on `isrt` button
  observeEvent(input$isrt, {
    
    # handle the case when user does not provide ID
    divID <- if (input$divID == "") "id" else input$divID
    btnID <- paste0(divID, "rmv")
    
    # only create button if there is none
    if (!rv$btnExists) {
      
      insertUI(
        selector = "#placeholder",
        ui = tags$div(id = divID, 
                      actionButton(btnID, "Remove this button"))
      )
      rv$btnExists <- TRUE
      print("created")
      
      # create a listener on the newly-created button that will
      # remove it from the app when clicked
      observeEvent(input[[btnID]], {
        removeUI(selector = paste0("#", divID))
        rv$btnExists <- FALSE
        print("destroyed")
        
      }, ignoreInit = TRUE, once = TRUE)
      
      # otherwise, print a message to the console
    } else {
      message("The button has already been created!")
    }
  })
}

shinyApp(ui = ui, server = server)

bborgesr added the review label Dec 5, 2016

bborgesr self-assigned this Dec 7, 2016

wch reviewed Dec 12, 2016

View reviewed changes

bborgesr added 10 commits December 19, 2016 23:12

added skipFirs arg to observeEvent

1513459

create getCurrentObserver() function

e076aba

better NEWS entry

812e816

made code more consistent

dd99130

implemented once param to observeEvent; extensive documentation f…

8430b24

…or `getCurrentObserver`

implement dig param to getCurrentObserver

c671383

fix bug that was causing unit tests to fail

b88d285

take two

5067afb

git commit

4ee1924

removed function getCurrentObserver

64e717e

bborgesr added 2 commits December 19, 2016 23:16

delete .globals$currentObserver variable

7a335b7

update docs

55b90f8

bborgesr force-pushed the barbara/observe branch from c938d86 to 55b90f8 Compare December 19, 2016 23:17

bborgesr added 3 commits December 19, 2016 23:19

typo

156bcf1

remove dupes in index.r (bah humbug)

4c51b4e

rerun devtools::document

c829112

jcheng5 merged commit 1962369 into master Dec 19, 2016

jcheng5 deleted the barbara/observe branch December 19, 2016 23:51

jcheng5 removed the review label Dec 19, 2016

fpgmaas mentioned this pull request Dec 14, 2017

Outdated documentation of eventReactive #1907

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added skipFirst arg to observeEvent #1494

Added skipFirst arg to observeEvent #1494

bborgesr commented Dec 5, 2016 •

edited

wch commented Dec 5, 2016

wch commented Dec 7, 2016

wch commented Dec 7, 2016

jcheng5 commented Dec 7, 2016

jcheng5 commented Dec 7, 2016 •

edited

bborgesr commented Dec 8, 2016

jcheng5 commented Dec 8, 2016

bborgesr commented Dec 8, 2016

jcheng5 commented Dec 8, 2016

bborgesr commented Dec 8, 2016

bborgesr commented Dec 8, 2016

jcheng5 commented Dec 8, 2016

wch Dec 12, 2016

wch Dec 12, 2016

wch Dec 12, 2016

wch Dec 12, 2016

wch Dec 12, 2016

wch Dec 12, 2016

wch Dec 12, 2016

jcheng5 Dec 13, 2016

bborgesr Dec 13, 2016

jcheng5 Dec 13, 2016

bborgesr Dec 13, 2016

bborgesr commented Dec 13, 2016

jcheng5 commented Dec 13, 2016

bborgesr commented Dec 13, 2016

jcheng5 commented Dec 13, 2016

bborgesr commented Dec 13, 2016

bborgesr commented Dec 19, 2016

Added skipFirst arg to observeEvent #1494

Added skipFirst arg to observeEvent #1494

Conversation

bborgesr commented Dec 5, 2016 • edited

wch commented Dec 5, 2016

wch commented Dec 7, 2016

wch commented Dec 7, 2016

jcheng5 commented Dec 7, 2016

jcheng5 commented Dec 7, 2016 • edited

bborgesr commented Dec 8, 2016

jcheng5 commented Dec 8, 2016

bborgesr commented Dec 8, 2016

jcheng5 commented Dec 8, 2016

bborgesr commented Dec 8, 2016

bborgesr commented Dec 8, 2016

jcheng5 commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bborgesr commented Dec 13, 2016

jcheng5 commented Dec 13, 2016

bborgesr commented Dec 13, 2016

jcheng5 commented Dec 13, 2016

bborgesr commented Dec 13, 2016

bborgesr commented Dec 19, 2016

bborgesr commented Dec 5, 2016 •

edited

jcheng5 commented Dec 7, 2016 •

edited