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

remove pingr dependency #1318

Closed
wants to merge 2 commits into from
Closed

remove pingr dependency #1318

wants to merge 2 commits into from

Conversation

Adafede
Copy link

@Adafede Adafede commented Aug 16, 2024

Prework

Related GitHub issues and pull requests

Summary

Hi @wlandau,

I try to solve the request raised in the above mentionned discussion.

@wlandau
Copy link
Member

wlandau commented Aug 19, 2024

The general approach here is great, not just for WASM, but also to shorten the long dependency list just a little bit. I implemented this in 409f991 with minor alterations and credited you in the NEWS.md file.

@wlandau wlandau closed this Aug 19, 2024
@@ -176,7 +176,16 @@ url_process_error <- function(url, req, headers) {
url_port <- function(host, port, process, verbose = TRUE) {
spin <- cli::make_spinner()
if_any(verbose, spin$spin(), NULL)
while (!pingr::is_up(destination = host, port = port)) {
.is_up <- function(host, port) {
con <- try(suppressWarnings(socketConnection(host, port, open = "r")), silent = TRUE)
Copy link
Member

@hadley hadley Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO a slightly clearer style here is something like:

tryCatch(
  {
    con <- suppressWarnings(socketConnection(host, port, open = "r"))
    close(con)
    TRUE
  },
  error = function(cnd) {
    FALSE
  }
)

(And note that this still won't work in webR)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And note that this still won't work in webR)

That seems okay because tar_watch() itself is only meant to be run locally. shinylive apps will be using the module in tar_watch_ui() and tar_watch_server(), which do not use pingr. (Although until today tar_watch_ui() and tar_watch_server() did still enforce all the same dependencies as tar_watch(), including pingr, which was an oversimplification.)

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

Successfully merging this pull request may close these issues.

3 participants