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

get rid of "sleep()" anti-pattern? #176

Open
cellocgw opened this issue Sep 11, 2024 · 3 comments
Open

get rid of "sleep()" anti-pattern? #176

cellocgw opened this issue Sep 11, 2024 · 3 comments

Comments

@cellocgw
Copy link

cellocgw commented Sep 11, 2024

I just read the blog entry https://ropensci.org/blog/2024/09/10/script-screenshots/ and noticed the infamous "anti-pattern" use of "sleep()" to hope&pray a call is complete. Isn't there a way to query some status item the Chrome instance to see that page loading is finished?

@gadenbuie
Copy link
Member

There is, and it's documented in Loading a page reliably in the README (or in the pkgdown site here).

That said, I think "anti-pattern" is too strong of a term. Yes there are methods that will better guarantee the page has loaded or move forward as soon as the page has loaded, but Sys.sleep() isn't unreasonable to use depending on the circumstances.

I do think we should make it easier to use the "wait for page load" pattern and have proposed a way that we could do that in #177.

@cellocgw
Copy link
Author

@gadenbuie thanks for the information -- I wish that had been included in the referenced blog post.
I'll stand by my dislike of "sleep()" or equivalents as a way to pseudo-wait for things to complete. I've seen too many failures related to similar implementations elsewhere. It's really good to know that the package does have a tool to test for completion.

@gadenbuie
Copy link
Member

gadenbuie commented Sep 12, 2024

I'll stand by my dislike of "sleep()" or equivalents as a way to pseudo-wait for things to complete. I've seen too many failures related to similar implementations elsewhere. It's really good to know that the package does have a tool to test for completion.

The primary tool we have in chromote is b$Page$loadEventFired(), which waits until the browser fires the load event. Unfortunately, this is often still too early for the page to be completely loaded.

You can see this play out with the following script

library(chromote)

b <- ChromoteSession$new()

b$Page$navigate("https://r-universe.dev/search/")
b$Page$loadEventFired()
b$screenshot("load_event.png")

b$Page$reload()
Sys.sleep(2)
b$screenshot("wait.png")

The first screenshot will not have the full page because there is content that is added to the page after the load event. There are certainly approaches that can be used to very confidently wait until a specific element is loaded or a browser event is fired or a network request finishes, but none of those are fun to program with the Chrome Developer Protocol's low-level features.

I have further suggestions in #177 but it's still reasonable in this case for someone to asses the website being loaded, their own network conditions and the behavior of the website and to decide to just ballpark reasonable values for a delay. By doing so, they trade hours of additional dev work that would only save maybe a few dozen seconds over the life of the code.

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

2 participants