Wait in click()

[jonthegeek]

Closes #405.

I tried to make this automatic, and got it working some of the time, but I couldn't find a way to detect that something might change, and thus wait in that situation. I tried to make it as clear as possible for users to be able to fix this. Once a strategy is agreed on, the same strategy should probably be applied to other methods.

An approach I worked on but failed to finalize would be to either wait for a loadEventFired() event or time out (probably after a relatively brief time, and only if no events occur in that window, or something along those lines). I couldn't find a clean way to check if anything is in progress and thus wait if so; if we're waiting for a loadEventFired that never fires, we'll introduced a new error for many click events.

I also tried adding private$refresh_root() as a callback on loadEventFired(), which might be a useful approach but it still doesn't solve the issue of users not waiting for that to occur before executing their next step.

I like this better than status quo (since it makes it possible to deal with these situations), but I'm not sure that it's quite there yet.

[hadley]

I think I'd rather make refresh_root() public and then suggest that the user call it. But I'd also want to be 100% sure we shouldn't just call refresh_root() on their behalf.

[jonthegeek]

The problem is it might not be ready yet. I feel like there has to be a way to essentially wait for the loadEventFired only if the page is navigating or reloading, but I couldn't get there.

[hadley]

Maybe we should always be waiting on the loadEventFired so we can update the root node.

[jonthegeek]

I did put refresh_root() into loadEventFired as a callback at one point, and that's probably worth re-implementing. The problem occurs when the user tries to do something that results in a find_nodes() after a click, but before everything has processed. If the click results in a loadEventFired() and we don't wait before find_nodes(), the root will be wrong (which triggered a thought, see below). But if we do wait when they click somewhere that doesn't navigate/reload, the loadEventFired() never occurs and times out, so we make them wait for nothing (and we throw a confusing warning or error).

But... something knows that the page is in an invalid state, because there's an error when you try to find_nodes() too soon. So in theory at THAT point waiting on loadEventFired() should work. I feel like that's getting into the issues that are discussed in https://rstudio.github.io/chromote/#loading-a-page-reliably though, so that might not solve it.

Perhaps create the promise right away on click (and anything else that might change the page), but only wait for it to resolve when there's an indication that it hasn't resolved? Ie, inside click we'd do something like this:

private$load_promise <- self$session$Page$loadEventFired(wait_ = FALSE)

And then in find_nodes() we could wait for that promise to resolve and try again (inside this try_fetch(), probably). Async programming still always feels weird to me, so I can't decide if this is hacky, correct, or somewhere in-between.

We might also have to capture the timeout message if the promise never resolves, to avoid random confusing warnings showing up for the user.

[hadley]

Thanks for the detailed explanation! I think that approach sounds like a way forward, and then I can double check it with Joe/Winston.

[jonthegeek]

It's closer now, I think, but it fails ungracefully for "normal" clicks, and I haven't yet found a way to clean this up. To see what I mean, run the "can click a button" test in test-live.R a few times. Or, more specifically:

sess <- read_html_live(html_test_path("click"))
sess$click("button")
Sys.sleep(10)
rm(sess)  

I can see where that happens in {chromote}, and it makes sense that it does so, but the only way I can think of to avoid the error message would be to set the timeout to Inf, and that just means we could stack up promises that never resolve. Or maybe line 149 of live.R should do something fancier when it sets up private$loadEvent_promise, to capture (and silently discard) the promise if it times out.