-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
feature: automatically launch browser on bud run
#334
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for opening this PR!
internal/cli/run/run.go
Outdated
if !c.Flag.Noautolaunch { // if not not autolaunch, i.e. if autolaunch | ||
go func() { | ||
time.Sleep(1 * time.Second) | ||
_ = browser.OpenURL("http://" + webln.Addr().String()) |
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.
Should check the error
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.
this is the only guidance here i don't understand. why? i literally can't think of anything that we'd do differently if it fails or not. it's an optional convenience feature...
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.
My take is that if a user explicitly requested --open
we should open or fail trying.
Thanks for the review and feedback- I will update the PR soon. |
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.
Looks a lot better, thanks the update!
internal/cli/cli.go
Outdated
@@ -63,6 +63,7 @@ func (c *CLI) Run(ctx context.Context, args ...string) error { | |||
cli.Flag("embed", "embed assets").Bool(&cmd.Flag.Embed).Default(false) | |||
cli.Flag("hot", "hot reloading").Bool(&cmd.Flag.Hot).Default(true) | |||
cli.Flag("minify", "minify assets").Bool(&cmd.Flag.Minify).Default(false) | |||
cli.Flag("open", "open browser on dev server startup").Bool(&cmd.Flag.OpenBrowser).Default(false) |
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.
Ah sorry, I might have missed this in the previous review but Flag is meant for global options that need to get passed into the framework. We should treat OpenBrowser
like Listen below and stick it on the run command.
abd4715
to
227541b
Compare
227541b
to
31e089c
Compare
31e089c
to
3ce87a8
Compare
fixes #333