-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat(cli.rs): wait for dev URL to be reachable, exit if command fails #3358
Conversation
tooling/cli.rs/src/dev.rs
Outdated
} | ||
}; | ||
let mut i = 0; | ||
let sleep_interval = std::time::Duration::from_secs(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 seconds is too long, don't you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm tricky. With vite you don't even get into a second iteration, because it's too fast.
With webpack taking like 30 seconds it would get quite spammy with timeouts < 5secs.
Maybe reduce the timeout, but only log the message every x tries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I was mainly trying to reduce the spam, since some frameworks like Quasar can take a minute to build. i'll reduce the logging instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed, let me know what you think
Co-authored-by: Amr Bashir <amr.bashir2015@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I can't test the ghost process issue though (currently away) but I posted the steps to try and reproduce it in the other PR.
Yeah the ghost process is part of a separate investigation and PR. |
Okay, This PR is fine as is then. |
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
fix: remove a typo, closes #___, #___
)Other information
Related to #2813